fix(work-apps): make Slack page toasts persist until manually closed#1958
fix(work-apps): make Slack page toasts persist until manually closed#1958amikofalvy wants to merge 1 commit intomainfrom
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Medium
🟠⚠️ Major (1) 🟠⚠️
🟠 1) multi-file Toast duration: Infinity creates inconsistent UX pattern across Management UI
files:
workspace-hero.tsx:166-172agent-configuration-card.tsx:331-371linked-users-section.tsx:77-106notification-banner.tsx
Issue: This PR introduces duration: Infinity for all toasts on Slack integration pages, making them persist until manually closed. This diverges from the established convention across the entire Management UI where toasts auto-dismiss using the default duration. The closest peer pages (GitHub Work App at /work-apps/github/) use default durations without any duration option. This creates a split-world where Slack toasts behave differently than every other page in the app, including the sibling GitHub integration in the same Work Apps section.
Why: Users navigating between pages in the Management UI will experience inconsistent toast behavior — toasts on the Slack page require manual dismissal while toasts everywhere else auto-dismiss. This inconsistency can confuse users and creates UX debt. Success toasts persisting indefinitely is also an unusual UX pattern — users typically expect success confirmations to auto-dismiss since they indicate "all good, nothing to do." Error toasts persisting is more defensible since users may need time to read and act on them.
Fix: Consider one of these approaches:
- Remove
duration: Infinityfrom all toast calls and use the default auto-dismiss behavior to maintain consistency with the rest of the Management UI - Differentiate by toast type: Keep error toasts persistent (
duration: Infinity) since they require user attention, but let success toasts auto-dismiss (default behavior) since they confirm completion - If persistent toasts are truly needed for Slack: Document the rationale and consider whether this should be a platform-wide pattern change. If applying only to Slack, also update the GitHub integration for Work Apps consistency
Refs:
- GitHub integration page (peer) uses default durations
- GitHub installation page uses default durations
- Triggers table uses default durations
💭 Consider (1) 💭
💭 1) notification-banner.tsx Consolidate notification mechanisms on Slack page
Issue: The Slack page uses two different notification systems: the NotificationBanner component (a sticky banner at the top using context state via useSlack()) and sonner toast() calls (the standard toast system used elsewhere in the app). After this PR, both persist indefinitely, but they serve different purposes and appear in different UI locations.
Why: Having two notification mechanisms increases cognitive load and may confuse users about which notification style applies when. It also increases maintenance burden.
Fix: Consider consolidating to a single notification approach for the Slack page — either use only sonner toasts (matching the rest of the app) or only the NotificationBanner (if sticky page-level context is truly needed for certain operations).
Refs:
💡 APPROVE WITH SUGGESTIONS
Summary: The code is correct and accomplishes the stated goal, but introduces an inconsistent UX pattern. The Slack page will now behave differently from every other page in the Management UI (including the sibling GitHub integration) regarding toast auto-dismiss behavior. Please consider either removing the duration: Infinity to maintain consistency, or documenting why Slack specifically needs different behavior and whether this pattern should be applied more broadly.
Discarded (0)
No findings were discarded.
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-consistency |
1 | 1 | 0 | 0 | 0 | 0 | 0 |
pr-review-product |
2 | 1 (merged) | 1 | 0 | 0 | 0 | 0 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-frontend |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 3 | 1 | 1 | 0 | 0 | 0 | 0 |
Note: The consistency and product reviewers both flagged the same inconsistency issue — findings were merged.
c839bd2 to
7e9cb81
Compare
The Slack integration page had two separate notification systems: a custom NotificationBanner (Zustand-backed, Slack-only) and sonner toasts (the app-wide standard used in 80+ files). This consolidates everything onto sonner so there is a single toast framework. - Replace all actions.setNotification() calls with toast.success/error/info() - Delete NotificationBanner component, Slack notification store, and types - Remove notification state from SlackProvider context - Error toasts use duration: Infinity so they persist until closed - Success/info toasts use the default auto-dismiss duration for consistency with the rest of the Management UI Co-authored-by: Cursor <cursoragent@cursor.com>
7e9cb81 to
87d2e5b
Compare
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Medium
💭 Consider (1) 💭
💭 1) PR description PR description doesn't match implemented behavior
Issue: The PR description states "Toasts and banners now persist until the user explicitly closes them via the close button" — but the implementation differentiates between error toasts (which use duration: Infinity and persist) and success/info toasts (which use default auto-dismiss).
Why: This is actually a reasonable UX pattern (errors deserve user attention, success messages can auto-dismiss), but the documentation doesn't reflect it. Future maintainers may be confused about whether the inconsistency is intentional.
Fix: Update the PR description to accurately describe the behavior: "Error toasts now persist until manually closed (duration: Infinity); success and info toasts continue to auto-dismiss using sonner's default behavior."
Refs:
- agent-configuration-card.tsx:178 — success toast with default duration
- agent-configuration-card.tsx:181 — error toast with
duration: Infinity
🕐 Pending Recommendations (1)
- 🟠
multi-fileToastduration: Infinitycreates inconsistent UX pattern across Management UI — error toasts on the Slack page now require manual dismissal while all other pages (including the sibling GitHub Work App) auto-dismiss errors
💡 APPROVE WITH SUGGESTIONS
Summary: The architectural consolidation is solid — removing the custom NotificationBanner component, Zustand store, and Slack-specific types in favor of the standard sonner toast library is a good simplification. No new code quality or correctness issues were found. The prior feedback about inconsistent duration: Infinity usage across the Management UI remains pending — please address or acknowledge the UX consistency question raised in the previous review before merging.
Discarded (2)
| Location | Issue | Reason Discarded |
|---|---|---|
agent-configuration-card.tsx:178 |
Success toast missing duration: Infinity |
Intentional design — only error toasts persist, success toasts auto-dismiss (reasonable UX pattern) |
slack-dashboard.tsx:37 |
Info toast for cancelled installation uses default duration | Intentional design — cancellation is user-initiated and doesn't need persistent acknowledgment |
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-consistency |
1 | 0 | 0 | 0 | 0 | 1 | 0 |
pr-review-product |
2 | 0 | 1 | 0 | 0 | 0 | 1 |
pr-review-frontend |
2 | 0 | 0 | 0 | 0 | 0 | 2 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 5 | 0 | 1 | 0 | 0 | 1 | 3 |
Note: The consistency reviewer's finding was the same issue raised in the prior review — routed to Pending Recommendations rather than re-raised.
Summary
NotificationBannercomponent on the Slack integration pageduration: Infinityto all sonner toast calls (toast.success/toast.error) acrossworkspace-hero,linked-users-section, andagent-configuration-cardTest plan
Made with Cursor