Skip to content
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.

Sign up for GitHub

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

fix: Add unsubscribe functionality to NewsletterModal #210

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

fix: Add unsubscribe functionality to NewsletterModal #210

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
44be712
fix: Add unsubscribe functionality to `NewsletterModal` (#209)
aryanprince Aug 11, 2024
2f53ad3
refactor: Update `NewsletterModal` usages throughout app
aryanprince Aug 11, 2024
1b7c1b8
refactor: Add button disabled state, use ternary logic instead
aryanprince Aug 23, 2024
6ff6a65
chore: Remove button loading state comments
aryanprince Sep 24, 2024
32a67b2
Merge remote-tracking branch 'origin/main' into fix/newsletter-modal-…
aryanprince Sep 24, 2024
ae22a6c
fix: Add missing `mutate` prop to `NewsletterModal`
aryanprince Sep 24, 2024
17ce178
fix: `refreshInterval` now accessed directly, pass `mutate` prop
aryanprince Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions apps/web/app/(app)/bulk-unsubscribe/BulkUnsubscribeSection.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,11 +223,14 @@ export function BulkUnsubscribeSection(props: {
</LoadingContent>
)}
</Card>
<NewsletterModal
newsletter={openedNewsletter}
onClose={() => setOpenedNewsletter(undefined)}
refreshInterval={props.refreshInterval}
/>
{openedNewsletter && (
Copy link
Owner

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?

Copy link
Author

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) the openedNewsletter prop, which was previously optional.

The reason for this change is that the component now uses the existing useUnsubscribeButton hook, which does need a required openedNewsletter prop.

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 openedNewsletter a 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.

CleanShot 2024-08-23 at 12 29 31

<NewsletterModal
newsletter={openedNewsletter}
onClose={() => setOpenedNewsletter(undefined)}
refreshInterval={props.refreshInterval}
mutate={mutate}
/>
)}
<PremiumModal />
</>
);
Expand Down
12 changes: 6 additions & 6 deletions apps/web/app/(app)/cold-email-blocker/ColdEmailList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,12 +143,12 @@ export function ColdEmailList() {

<TablePagination totalPages={data.totalPages} />

<NewsletterModal
newsletter={
openedRow ? { name: openedRow.fromEmail || "" } : undefined
}
onClose={() => setOpenedRow(undefined)}
/>
{openedRow && (
<NewsletterModal
newsletter={openedRow && { name: openedRow.fromEmail || "" }}
onClose={() => setOpenedRow(undefined)}
/>
)}
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

Add Missing mutate Prop to NewsletterModal in ColdEmailList.tsx

The NewsletterModal in ColdEmailList.tsx is missing the mutate prop, which is necessary for handling the unsubscribe functionality as outlined in the PR objectives. To ensure consistent behavior across all components, please add the mutate prop as shown below:

 {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 chain

Simplify newsletter prop and add missing mutate prop

The conditional rendering of NewsletterModal is an improvement, but there are a couple of points to address:

  1. The newsletter prop can be simplified since openedRow is already checked in the conditional rendering.
  2. The mutate prop is missing, which is mentioned in the PR objectives as part of the fix for the unsubscribe functionality issue.

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 NewsletterModal component (not visible in this file), fully address the unsubscribe functionality issue mentioned in the PR objectives? If not, what additional changes are needed?

Scripts executed

The 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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion, resolved!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?
Thank you for using CodeRabbit!

</div>
) : (
<NoColdEmails />
Expand Down
12 changes: 6 additions & 6 deletions apps/web/app/(app)/cold-email-blocker/ColdEmailRejected.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,12 @@ export function ColdEmailRejected() {

<TablePagination totalPages={data.totalPages} />

<NewsletterModal
newsletter={
openedRow ? { name: openedRow.fromEmail || "" } : undefined
}
onClose={() => setOpenedRow(undefined)}
/>
{openedRow && (
<NewsletterModal
newsletter={openedRow && { name: openedRow.fromEmail || "" }}
onClose={() => setOpenedRow(undefined)}
/>
)}
</div>
) : (
<NoRejectedColdEmails />
Expand Down
12 changes: 7 additions & 5 deletions apps/web/app/(app)/new-senders/NewSenders.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,11 +211,13 @@ export function NewSenders(props: {
<div className="mt-2 px-6 pb-6">{extra}</div>
</LoadingContent>
</Card>
<NewsletterModal
newsletter={openedNewsletter}
onClose={() => setOpenedNewsletter(undefined)}
refreshInterval={props.refreshInterval}
/>
{openedNewsletter && (
<NewsletterModal
newsletter={openedNewsletter}
onClose={() => setOpenedNewsletter(undefined)}
refreshInterval={props.refreshInterval}
/>
)}
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Inconsistent prop access and missing mutate prop.

There are two issues with the current implementation:

  1. The refreshInterval prop is accessed via props.refreshInterval, which is inconsistent with the component's parameter destructuring. It should be accessed directly as refreshInterval.

  2. The PR objectives mention passing a mutate prop to the NewsletterModal, but this is not visible in the changes. This prop is crucial for addressing the unsubscribe functionality bug.

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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{openedNewsletter && (
<NewsletterModal
newsletter={openedNewsletter}
onClose={() => setOpenedNewsletter(undefined)}
refreshInterval={props.refreshInterval}
/>
)}
{openedNewsletter && (
<NewsletterModal
newsletter={openedNewsletter}
onClose={() => setOpenedNewsletter(undefined)}
refreshInterval={refreshInterval}
mutate={mutate}
/>
)}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed it now!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aryanprince, glad to hear you've fixed it!

(^‿^)/

If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

<PremiumModal />
</>
);
Expand Down
34 changes: 30 additions & 4 deletions apps/web/app/(app)/stats/NewsletterModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,27 +21,46 @@ import { SectionHeader } from "@/components/Typography";
import { EmailList } from "@/components/email-list/EmailList";
import type { ThreadsResponse } from "@/app/api/google/threads/controller";
import { Tabs, TabsContent, TabsList, TabsTrigger } from "@/components/ui/tabs";
import { Button } from "@/components/ui/button";
import { Button, ButtonLoader } from "@/components/ui/button";
import { getGmailFilterSettingsUrl } from "@/utils/url";
import { Tooltip } from "@/components/Tooltip";
import { AlertBasic } from "@/components/Alert";
import { onAutoArchive } from "@/utils/actions/client";
import { MoreDropdown } from "@/app/(app)/bulk-unsubscribe/common";
import {
MoreDropdown,
useUnsubscribeButton,
} from "@/app/(app)/bulk-unsubscribe/common";
import { useLabels } from "@/hooks/useLabels";
import { Row } from "@/app/(app)/bulk-unsubscribe/types";
import { usePremium } from "@/components/PremiumAlert";
import { usePostHog } from "posthog-js/react";

export function NewsletterModal(props: {
newsletter?: Pick<Row, "name" | "lastUnsubscribeLink" | "autoArchived">;
newsletter: Pick<Row, "name" | "lastUnsubscribeLink" | "autoArchived">;
onClose: (isOpen: boolean) => void;
refreshInterval?: number;
mutate?: () => Promise<any>;
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid Using any Type

The use of any should be avoided to maintain type safety. Consider specifying a more precise type for the mutate function to enhance type checking.

- mutate?: () => Promise<any>;
+ mutate?: () => Promise<void>;
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
mutate?: () => Promise<any>;
mutate?: () => Promise<void>;
Tools
Biome

[error] 42-42: Unexpected any. Specify a different type.

any disables many type checking rules. Its use should be avoided.

(lint/suspicious/noExplicitAny)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree bot. I tried my best to make the suggestion of mutate?: () => Promise<void>; work but there were a lot of side effects with it. Would be happy to see a fix for this.

The issue here is that the mutate function passed down to this NewsletterModal component is to mutate multiple newsletters, but this component only needs to mutate a single newsletter.

The only fixes I could think of was to:

  1. Create a new API route to mutate a single newsletter as well, along with the existing API route to mutate multiple newsletters.
  2. Update the useUnsubscribeButton hook's logic, and all of its usages.

I think this will need refactoring in the future regardless. Would be happy to have a look at this later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 mutate function. To ensure this doesn't get lost, would you like me to create a follow-up issue to track this refactoring task? This way, we can revisit it when the time is right and ensure the NewsletterModal component handles mutations more precisely. Let me know how you'd like to proceed!

Comment on lines +37 to +40
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Approve newsletter prop change, but consider specifying mutate type

The change to make the newsletter prop required is good as it ensures the component always receives necessary data. However, for the mutate prop:

The newsletter prop change is approved.

The mutate prop uses the any type, which is generally discouraged as it bypasses TypeScript's type checking. Consider specifying a more precise return type for the Promise. If the exact type is unknown, you could use unknown instead of any:

mutate?: () => Promise<unknown>;

This maintains type safety while allowing flexibility in the return type.

Tools
Biome

[error] 40-40: Unexpected any. Specify a different type.

any disables many type checking rules. Its use should be avoided.

(lint/suspicious/noExplicitAny)

}) {
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 } = useUnsubscribeButton({
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]">
Expand All @@ -57,8 +76,15 @@ export function NewsletterModal(props: {
href={newsletter.lastUnsubscribeLink || undefined}
target="_blank"
rel="noreferrer"
onClick={onUnsubscribe}
>
Unsubscribe
{!unsubscribeLoading && <span>Unsubscribe</span>}
{!!unsubscribeLoading && (
<div className="flex cursor-not-allowed items-center opacity-50">
<ButtonLoader />
<span>Unsubscribing...</span>
</div>
)}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use a ternary to make this cleaner. ie: unsubscribeLoading ? Unsub : Loading...

If you're trying to add disabled state, then you could add disabled={unsubscribeLoading} to the button itself.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated it now! Used a ternary here instead and added the disabled state to the button.

</a>
</Button>
<Tooltip content="Auto archive emails using Gmail filters">
Expand Down