Add slack connection banner to sidebar #1870
Conversation
Co-authored-by: yul.schnay <[email protected]>
|
Cursor Agent can help with this pull request. Just |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughIntroduces a Slack sidebar banner prompting setup, wires it into the sidebar with cookie-based dismissal and status check, adds analytics events on banner and settings interactions, updates Slack OAuth callback to append success=true on redirect, and instruments the settings page to capture success/failure/connect events. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Sidebar as Sidebar
participant Banner as SlackBanner
participant Router as Next Router
participant Settings as Settings Page
participant OAuth as Slack OAuth
participant Callback as /api/.../slack/oauth/callback
User->>Sidebar: Open app
Sidebar->>Banner: Render if no Slack integration and cookie allows
User->>Banner: Click "Set up Slack"
Banner->>Router: navigate /settings/slack
Router->>Settings: Load Settings (Slack)
note over Settings: User clicks Connect
Settings->>OAuth: Start OAuth flow
OAuth-->>Callback: Redirect with code/state
Callback->>Router: redirect /settings/slack?success=true
Router->>Settings: Reload with success=true
Settings->>Settings: Capture "Slack Connected"
alt Error during OAuth
Settings->>Settings: Capture "Slack Connection Failed" (error)
end
User->>Banner: Dismiss banner (optional)
Banner->>Banner: Set cookie hideSlackBanner=slack-banner (30d)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/api/integrations/slack/oauth/callback/route.ts (1)
87-92: Return redirects on errors instead of JSON to preserve UX and analytics.Currently, failures return JSON from the OAuth callback, which strands users on a JSON page and skips client‑side toasts/analytics in
pages/settings/slack.tsx. Redirect with anerrorquery to unify flows.Apply this diff in the shown blocks:
- return NextResponse.json( - { error: `Slack OAuth error: ${data?.error || "unknown"}` }, - { status: 400 }, - ); + redirect(`/settings/slack?error=${encodeURIComponent(data?.error || "oauth_error")}`);- return NextResponse.json({ error: e.message }, { status: 500 }); + redirect(`/settings/slack?error=${encodeURIComponent("internal_error")}`);- return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); + redirect(`/settings/slack?error=unauthorized`);- return NextResponse.json({ error: "Invalid state" }, { status: 400 }); + redirect(`/settings/slack?error=invalid_state`);Also applies to: 110-112, 33-34, 45-46
🧹 Nitpick comments (10)
app/api/integrations/slack/oauth/callback/route.ts (2)
64-66: Remove unreachable null check afterfindUniqueOrThrow.
findUniqueOrThrowthrows when not found, soif (!team)can never be true and is dead code.- if (!team) { - return NextResponse.json({ error: "Access denied" }, { status: 403 }); - }
40-41: Fix stale comment.Comment references “Stripe app install” in a Slack OAuth handler. Update for clarity.
- // Find workspace that initiated the Stripe app install + // Find team that initiated the Slack app installpages/settings/slack.tsx (4)
70-73: Guard againstchannelsbeing undefined to avoid runtime crash.
channelscan be undefined before SWR resolves. Default to[].- const filteredChannels = useMemo( - () => channels.filter((channel) => !channel.is_archived), - [channels], - ); + const filteredChannels = useMemo( + () => (channels || []).filter((channel) => !channel.is_archived), + [channels], + );
92-93: Use portable timeout type to satisfy both DOM and Node typings.- let timeoutId: NodeJS.Timeout | null = null; + let timeoutId: ReturnType<typeof setTimeout> | null = null;
230-231: Remove unused timing variables.
startTime/endTimeare not used.- const startTime = performance.now(); ... - const endTime = performance.now();Also applies to: 240-240
293-298: Either implement a real debounce or rename to reflect immediate updates.Right now
debouncedChannelsUpdateisn’t debounced. Either wire a debounce (e.g.,useMemo+setTimeout/lodash.debounce) or rename tohandleChannelsChangefor accuracy.Also applies to: 522-533
components/sidebar/app-sidebar.tsx (2)
68-72: Avoid banner flicker by gating on integration loading.While SWR resolves,
slackIntegrationisnull, so the banner can flash briefly. Gate rendering on the hook’sloadingflag.- const { integration: slackIntegration } = useSlackIntegration({ + const { integration: slackIntegration, loading: loadingSlackIntegration } = useSlackIntegration({ enabled: !!currentTeam?.id, });- {!slackIntegration && showSlackBanner ? ( + {!loadingSlackIntegration && !slackIntegration && showSlackBanner ? ( <SlackBanner setShowSlackBanner={setShowSlackBanner} /> ) : null}Also applies to: 233-235
84-88: Consider scoping the “hide Slack banner” cookie per team.A single
hideSlackBannercookie hides the banner across all teams. If you want per‑team control, key it by team id (e.g.,hideSlackBanner:<teamId>) and pass the team id toSlackBannerto write the same key. Happy to provide a patch if you want this change now.components/sidebar/banners/slack-banner.tsx (2)
54-54: Tiny copy tweak.Grammatically smoother:
- <p className="my-4 text-sm">Get visit notifications in Slack channel.</p> + <p className="my-4 text-sm">Get visit notifications in a Slack channel.</p>
14-18: Optional: scope hide cookie per team to avoid cross‑team suppression.If desired, accept an optional
teamIdand include it in the cookie key.-export default function SlackBanner({ - setShowSlackBanner, -}: { - setShowSlackBanner: Dispatch<SetStateAction<boolean | null>>; -}) { +export default function SlackBanner({ + setShowSlackBanner, + teamId, +}: { + setShowSlackBanner: Dispatch<SetStateAction<boolean | null>>; + teamId?: string | null; +}) {- const handleHideBanner = () => { + const handleHideBanner = () => { setShowSlackBanner(false); plausible("clickedHideSlackBanner"); - Cookies.set("hideSlackBanner", "slack-banner", { + const cookieKey = teamId ? `hideSlackBanner:${teamId}` : "hideSlackBanner"; + Cookies.set(cookieKey, "slack-banner", { expires: 30, // Hide for 30 days }); };If you take this, pass
teamId={currentTeam?.id}from the sidebar where the banner is rendered.Also applies to: 23-29
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/api/integrations/slack/oauth/callback/route.ts(1 hunks)components/sidebar/app-sidebar.tsx(5 hunks)components/sidebar/banners/slack-banner.tsx(1 hunks)pages/settings/slack.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
components/sidebar/banners/slack-banner.tsx (1)
components/shared/icons/slack-icon.tsx (1)
SlackIcon(8-36)
components/sidebar/app-sidebar.tsx (2)
lib/swr/use-slack-integration.ts (1)
useSlackIntegration(8-29)components/sidebar/banners/slack-banner.tsx (1)
SlackBanner(14-67)
🔇 Additional comments (2)
app/api/integrations/slack/oauth/callback/route.ts (1)
114-114: Success redirect aligns with settings flow.Redirecting to
/settings/slack?success=truematches the client logic that toasts and tracks “Slack Connected”. Good change.pages/settings/slack.tsx (1)
98-103: Client analytics hooks look correct. VerifyuseAnalytics()is stable.If
useAnalytics()returns a new instance on re‑renders, the effect could retrigger. If it’s stable (memoized), you’re good.Would you confirm
useAnalyticsreturns a stable reference?Also applies to: 114-120
Add a dismissible Slack connection banner to the sidebar to encourage users to integrate Slack for notifications.
Slack Thread
Summary by CodeRabbit