fix: Add unsubscribe functionality to NewsletterModal
#210
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -144,12 +144,12 @@ export function ColdEmailList() { | |
|
|
||
| <TablePagination totalPages={data.totalPages} /> | ||
|
|
||
| {openedRow && ( | ||
| <NewsletterModal | ||
| newsletter={openedRow && { name: openedRow.fromEmail || "" }} | ||
| onClose={() => setOpenedRow(undefined)} | ||
| /> | ||
| )} | ||
|
There was a problem hiding this comment. 💡 Codebase verification Add Missing The {openedRow && (
<NewsletterModal
- newsletter={openedRow && { name: openedRow.fromEmail || "" }}
+ newsletter={{ name: openedRow.fromEmail || "" }}
onClose={() => setOpenedRow(undefined)}
+ mutate={mutate}
/>
)}This addition aligns with the implementation in other components and ensures the unsubscribe functionality operates correctly. Analysis chainSimplify The conditional rendering of
Please apply the following changes: {openedRow && (
<NewsletterModal
- newsletter={openedRow && { name: openedRow.fromEmail || "" }}
+ newsletter={{ name: openedRow.fromEmail || "" }}
onClose={() => setOpenedRow(undefined)}
+ mutate={mutate}
/>
)}Can you confirm if these changes, along with updates in the Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for NewsletterModal usage and implementation
rg --type typescript -A 10 'NewsletterModal'
Length of output: 103 Script: #!/bin/bash
# Search for NewsletterModal usage and implementation in .ts and .tsx files
rg 'NewsletterModal' --glob '*.ts*' -A 10
Length of output: 9199 There was a problem hiding this comment. @aryanprince, happy to hear that the issue has been resolved! (_/) If you found this review helpful, would you consider giving us a shout-out on X? |
||
| </div> | ||
| ) : ( | ||
| <NoColdEmails /> | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -211,11 +211,13 @@ export function NewSenders({ refreshInterval }: { refreshInterval: number }) { | |||||||||||||||||||||||||||||||
| <div className="mt-2 px-6 pb-6">{extra}</div> | ||||||||||||||||||||||||||||||||
| </LoadingContent> | ||||||||||||||||||||||||||||||||
| </Card> | ||||||||||||||||||||||||||||||||
| {openedNewsletter && ( | ||||||||||||||||||||||||||||||||
| <NewsletterModal | ||||||||||||||||||||||||||||||||
| newsletter={openedNewsletter} | ||||||||||||||||||||||||||||||||
| onClose={() => setOpenedNewsletter(undefined)} | ||||||||||||||||||||||||||||||||
| refreshInterval={props.refreshInterval} | ||||||||||||||||||||||||||||||||
| /> | ||||||||||||||||||||||||||||||||
| )} | ||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Inconsistent prop access and missing There are two issues with the current implementation:
To address these issues, please apply the following changes: {openedNewsletter && (
<NewsletterModal
newsletter={openedNewsletter}
onClose={() => setOpenedNewsletter(undefined)}
- refreshInterval={props.refreshInterval}
+ refreshInterval={refreshInterval}
+ mutate={mutate}
/>
)}Committable suggestion
Suggested change
There was a problem hiding this comment. @aryanprince, glad to hear you've fixed it! (^‿^)/ If you found this review helpful, would you consider giving us a shout-out on X? |
||||||||||||||||||||||||||||||||
| <PremiumModal /> | ||||||||||||||||||||||||||||||||
| </> | ||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,50 +1,64 @@ | ||||||
| import { MoreDropdown } from "@/app/(app)/bulk-unsubscribe/common"; | ||||||
| import { useUnsubscribe } from "@/app/(app)/bulk-unsubscribe/hooks"; | ||||||
| import { Row } from "@/app/(app)/bulk-unsubscribe/types"; | ||||||
| import { getDateRangeParams } from "@/app/(app)/stats/params"; | ||||||
| import type { ThreadsResponse } from "@/app/api/google/threads/controller"; | ||||||
| import type { | ||||||
| SenderEmailsQuery, | ||||||
| SenderEmailsResponse, | ||||||
| } from "@/app/api/user/stats/sender-emails/route"; | ||||||
| import { AlertBasic } from "@/components/Alert"; | ||||||
| import { LoadingContent } from "@/components/LoadingContent"; | ||||||
| import { usePremium } from "@/components/PremiumAlert"; | ||||||
| import { Tooltip } from "@/components/Tooltip"; | ||||||
| import { SectionHeader } from "@/components/Typography"; | ||||||
| import { EmailList } from "@/components/email-list/EmailList"; | ||||||
| import { Button } from "@/components/ui/button"; | ||||||
| import { | ||||||
| Dialog, | ||||||
| DialogContent, | ||||||
| DialogHeader, | ||||||
| DialogTitle, | ||||||
| } from "@/components/ui/dialog"; | ||||||
| import { Tabs, TabsContent, TabsList, TabsTrigger } from "@/components/ui/tabs"; | ||||||
| import { useLabels } from "@/hooks/useLabels"; | ||||||
| import { onAutoArchive } from "@/utils/actions/client"; | ||||||
| import { getGmailFilterSettingsUrl } from "@/utils/url"; | ||||||
| import type { ZodPeriod } from "@inboxzero/tinybird"; | ||||||
| import { BarChart } from "@tremor/react"; | ||||||
| import { ExternalLinkIcon } from "lucide-react"; | ||||||
| import { useSession } from "next-auth/react"; | ||||||
| import Link from "next/link"; | ||||||
| import { usePostHog } from "posthog-js/react"; | ||||||
| import type { DateRange } from "react-day-picker"; | ||||||
| import useSWR from "swr"; | ||||||
|
|
||||||
| export function NewsletterModal(props: { | ||||||
| newsletter: Pick<Row, "name" | "lastUnsubscribeLink" | "autoArchived">; | ||||||
| onClose: (isOpen: boolean) => void; | ||||||
| refreshInterval?: number; | ||||||
| mutate?: () => Promise<any>; | ||||||
|
There was a problem hiding this comment. Avoid Using The use of - mutate?: () => Promise<any>;
+ mutate?: () => Promise<void>;Committable suggestion
Suggested change
ToolsBiome
There was a problem hiding this comment. I agree bot. I tried my best to make the suggestion of The issue here is that the The only fixes I could think of was to:
I think this will need refactoring in the future regardless. Would be happy to have a look at this later. There was a problem hiding this comment. @aryanprince, thanks for sharing your insights and potential solutions. It seems like a refactoring effort could indeed be beneficial to address the scope of the
Comment on lines
+37
to
+40
There was a problem hiding this comment. Approve newsletter prop change, but consider specifying mutate type The change to make the The The mutate?: () => Promise<unknown>;This maintains type safety while allowing flexibility in the return type. ToolsBiome
|
||||||
| }) { | ||||||
| const { newsletter, refreshInterval, onClose } = props; | ||||||
| const mutate = props.mutate || (() => Promise.resolve()); // Set a default value for mutate if it's not provided | ||||||
|
|
||||||
| const session = useSession(); | ||||||
| const email = session.data?.user.email; | ||||||
|
|
||||||
| const { userLabels } = useLabels(); | ||||||
|
|
||||||
| const { hasUnsubscribeAccess, mutate: refetchPremium } = usePremium(); | ||||||
|
|
||||||
| const posthog = usePostHog(); | ||||||
|
|
||||||
| const { unsubscribeLoading, onUnsubscribe } = useUnsubscribe({ | ||||||
| item: newsletter, | ||||||
| hasUnsubscribeAccess, | ||||||
| mutate, | ||||||
| posthog, | ||||||
| refetchPremium, | ||||||
| }); | ||||||
|
|
||||||
| return ( | ||||||
| <Dialog open={!!newsletter} onOpenChange={onClose}> | ||||||
| <DialogContent className="max-h-screen overflow-x-scroll overflow-y-scroll lg:min-w-[880px] xl:min-w-[1280px]"> | ||||||
|
|
@@ -55,13 +69,21 @@ export function NewsletterModal(props: { | |||||
| </DialogHeader> | ||||||
|
|
||||||
| <div className="flex space-x-2"> | ||||||
| <Button size="sm" variant="outline" disabled={unsubscribeLoading}> | ||||||
| <a | ||||||
| href={newsletter.lastUnsubscribeLink || undefined} | ||||||
| target="_blank" | ||||||
| rel="noreferrer" | ||||||
| onClick={onUnsubscribe} | ||||||
| > | ||||||
| {unsubscribeLoading ? ( | ||||||
| <div className="flex cursor-not-allowed items-center opacity-50"> | ||||||
| <Button loading={true} /> | ||||||
| <span>Unsubscribing...</span> | ||||||
| </div> | ||||||
| ) : ( | ||||||
| <span>Unsubscribe</span> | ||||||
| )} | ||||||
| </a> | ||||||
| </Button> | ||||||
| <Tooltip content="Auto archive emails using Gmail filters"> | ||||||
|
|
||||||
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.
Did you need to add
{openedNewsletter && (?I think it stops the animation when it's done like that. Or was this added to fix a bug?
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.
Reason
This is cus the
<NewsletterModal />component now requires (non-optional) theopenedNewsletterprop, which was previously optional.The reason for this change is that the component now uses the existing
useUnsubscribeButtonhook, which does need a requiredopenedNewsletterprop.Initially, I tried keeping openedNewsletter as optional (to have less diffs like the one you brought up here), but that would have required introducing a second hook for unsubscribing, which would complicate the logic unnecessarily.
Minor Benefit
Additionally, making
openedNewslettera required prop also ensures that the<NewsletterModal />is only rendered when all necessary data is present, making it more robust and predictable in terms of when it renders throughout the app.Quick Demo
Also, below GIF shows that the animations are still intact. No issues with that according to my testing.