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

Add mail labels #355

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

Add mail labels #355

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
743c3d9
feat: add label handling in EmailListItem component
RicSala Feb 23, 2025
32f1db4
Merge branch 'main' into add-mail-labels
RicSala Feb 23, 2025
8b128ee
Fix label type: color may be undefined
RicSala Feb 23, 2025
14f36f6
fix: update import type for UserLabel and simplify threadLabels logic
RicSala Feb 23, 2025
76fcc06
refactor: Extract EmailLabels component
RicSala Feb 23, 2025
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
76 changes: 76 additions & 0 deletions apps/web/components/email-list/EmailListItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,13 @@ import { Button } from "@/components/ui/button";
import { findCtaLink } from "@/utils/parse/parseHtml.client";
import { ErrorBoundary } from "@/components/ErrorBoundary";
import { internalDateToDate } from "@/utils/date";
import { useLabels, UserLabel } from "@/hooks/useLabels";
import { Badge } from "@/components/ui/badge";
import { MoreVertical } from "lucide-react";
import { HoverCard } from "@/components/HoverCard";
import { isDefined } from "@/utils/types";

const MAX_THREAD_LABELS = 3;

export const EmailListItem = forwardRef(
(
Expand Down Expand Up @@ -71,6 +78,22 @@ export const EmailListItem = forwardRef(

const cta = findCtaLink(lastMessage.textHtml);

const { userLabels } = useLabels();
Copy link
Owner

Choose a reason for hiding this comment

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

so to optimise here, don't do uselabels on every row item. but do it a level above, and pass in the labels. basically any logic here is being run on every single row. and you could just have it run once for the table

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we are using SWR, actually I think only one request will be made! Others will be dedup (afaik).

I can move it to the parent, just wanted to avoid cluttering more the attributes of EmailListItem comp!

Copy link
Owner

Choose a reason for hiding this comment

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

Just asked Cursor. What it had to say. I considered your approach too, but I agree with the AI 😂

Cursor:

Right.Let me analyze this PR discussion about the useLabels hook in EmailListItem:

Both perspectives have valid points, but I would side with @elie222's suggestion for these reasons:

  1. Component Responsibility

    • Even though SWR will deduplicate the actual API requests, the hook execution itself still runs for every row item
    • The EmailListItem component should ideally be focused on presentation, not data fetching
    • Moving data fetching up reduces the cognitive load when reading the item component

  2. Performance Optimization

    • While SWR handles API deduplication, the hook still:
      • Creates new state instances per row
      • Runs its internal logic for each row
      • Triggers re-renders on each row when data changes
    • With many emails in the list, this overhead adds up

  3. Props vs. Hooks

    • Props make dependencies explicit and easier to test
    • Props allow better control over when re-renders happen
    • The parent component can optimize when labels need to be refetched

Here's how I would refactor it:

// ... existing code ...
const { userLabels } = useLabels();

return (
  <div>
    {threads.map(thread => (
      <EmailListItem 
        key={thread.id}
        userLabels={userLabels}
        thread={thread}
        // ... other props ...
      />
    ))}
  </div>
);
// ... existing code ...
export const EmailListItem = forwardRef(
  (
    props: {
      userLabels: Label[];  // Add this prop
      userEmailAddress: string;
      thread: Thread;
      // ... other props ...
    },
    ref: ForwardedRef<HTMLLIElement>,
  ) => {
    // Remove useLabels() hook
    // ... rest of component ...
  }
);

While the extra prop might seem like clutter, it's a worthwhile tradeoff for better performance and clearer component responsibilities. The parent component is a more appropriate place for data fetching that affects multiple child components.

Copy link
Owner

Choose a reason for hiding this comment

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

What I asked it btw:

what do you think about this discussion on a pr?

apps/web/components/email-list/EmailListItem.tsx
@@ -71,6 +76,21 @@ export const EmailListItem = forwardRef(

const cta = findCtaLink(lastMessage.textHtml);

const { userLabels } = useLabels();

Owner
@elie222 elie222 yesterday
so to optimise here, don't do uselabels on every row item. but do it a level above, and pass in the labels. basically any logic here is being run on every single row. and you could just have it run once for the table

Contributor
Author
@RicSala RicSala yesterday
As we are using SWR, actually I think only one request will be made! Others will be dedup (afaik).

I can move it to the parent, just wanted to avoid cluttering more the attributes of EmailListItem comp!


const threadLabels = useMemo(() => {
return thread.messages
.map((message) =>
message.labelIds
?.map((id) => userLabels?.find((label) => label.id === id))
.filter(Boolean),
)
.flat()
.filter(isDefined);
}, [thread.messages, userLabels]);

const hasLabels = threadLabels.length > 0;
const hasMoreLabels = threadLabels.length > MAX_THREAD_LABELS;

return (
<ErrorBoundary extra={{ props, cta, decodedSnippet }}>
<li
Expand Down Expand Up @@ -137,6 +160,32 @@ export const EmailListItem = forwardRef(
</Link>
</Button>
)}
{hasLabels && (
<LabelsGroup
labels={threadLabels}
maxShown={MAX_THREAD_LABELS}
/>
)}
{hasMoreLabels && (
<HoverCard
content={<LabelsGroup labels={threadLabels} wraps />}
>
<Button
variant="ghost"
size="icon"
className="w-fit px-2 hover:bg-transparent"
onClick={(e) => {
e.stopPropagation();
e.preventDefault();
}}
>
<MoreVertical
size={16}
className="text-muted-foreground"
/>
</Button>
</HoverCard>
)}
Copy link
Owner

Choose a reason for hiding this comment

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

make this whole thing a component. i think all the new code you added can be a component

Copy link
Owner

Choose a reason for hiding this comment

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

can email move it into its own file in the email-list folder

Copy link
Owner

Choose a reason for hiding this comment

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

possible we reuse this in other places in the app

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

btw, I used the badge from /ui folder because the other one (the one in components) was a bit rigid for the gmail color schemas.

I could either:

1- leave it as it is (using /ui/badge)
2- make /components/badge more flexible (just a bit concerned about giving it "too much flexibility"), for example adding a style attr
3- Creating a "emailLabel" component, that uses the shadcn badge (ui/badge), and consistently use that one for email labels
4- Add the rest of the colors to badgeVariants

In my opinion, 1 and 3 are the cleanest, but lmk if you think otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elie222 ☝🏻

<div className="ml-2 min-w-0 overflow-hidden text-foreground">
{lastMessage.headers.subject}
</div>
Expand Down Expand Up @@ -217,3 +266,30 @@ export const EmailListItem = forwardRef(
);

EmailListItem.displayName = "EmailListItem";

const LabelsGroup = ({
labels,
maxShown,
wraps = false,
}: {
labels: UserLabel[];
maxShown?: number;
wraps?: boolean;
}) => {
return (
<div className={clsx("ml-2 flex gap-2", { "flex-wrap": wraps })}>
{labels.slice(0, maxShown).map((label) => (
<Badge
className=""
key={label.id}
style={{
color: label?.color.textColor,
backgroundColor: label?.color.backgroundColor,
}}
>
{label.name}
</Badge>
))}
</div>
);
};
10 changes: 9 additions & 1 deletion apps/web/hooks/useLabels.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,15 @@ import useSWR from "swr";
import type { LabelsResponse } from "@/app/api/google/labels/route";
import type { gmail_v1 } from "@googleapis/gmail";

export type UserLabel = { id: string; name: string; type: "user" };
export type UserLabel = {
id: string;
name: string;
type: "user";
color: {
textColor: string;
backgroundColor: string;
};
};

export function useLabels() {
const { data, isLoading, error, mutate } =
Expand Down