Conversation
|
@RicSala is attempting to deploy a commit to the Inbox Zero Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes enhance the email thread display by integrating user label functionality. In the email list component, a constant limits the number of thread labels, and the Changes
Sequence Diagram(s)sequenceDiagram
participant EL as EmailListItem
participant UL as useLabels Hook
participant UM as useMemo Processing
participant LG as EmailLabels Component
participant UI as UI Renderer
EL->>UL: Retrieve userLabels
EL->>UM: Process thread messages for labels (useMemo)
EL->>LG: Render EmailLabels if labels exist
alt More labels than allowed
EL->>UI: Wrap EmailLabels in a HoverCard
end
LG->>UI: Render each label as a Badge with color styling
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
apps/web/components/email-list/EmailListItem.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Failed to load parser '@typescript-eslint/parser' declared in 'apps/web/.eslintrc.json': Cannot find module '@typescript-eslint/parser'
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (3)
apps/web/components/email-list/EmailListItem.tsx (3)
24-24: Useimport typefor type-only imports.The
UserLabelimport is only used as a type and should be imported using thetypekeyword.-import { useLabels, UserLabel } from "@/hooks/useLabels"; +import { useLabels } from "@/hooks/useLabels"; +import type { UserLabel } from "@/hooks/useLabels";🧰 Tools
🪛 Biome (1.9.4)
[error] 24-24: Some named imports are only used as types.
This import is only used as a type.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Add inline type keywords.(lint/style/useImportType)
83-92: Optimize array operations usingflatMap.The chain of
.map().flat()can be simplified using.flatMap()for better readability and performance.const threadLabels = useMemo(() => { - return thread.messages - .map((message) => + return thread.messages.flatMap((message) => message.labelIds ?.map((id) => userLabels?.find((label) => label.id === id)) - .filter(Boolean), - ) - .flat() + .filter(Boolean) ?? [] + ) .filter(isDefined); }, [thread.messages, userLabels]);🧰 Tools
🪛 Biome (1.9.4)
[error] 84-90: The call chain .map().flat() can be replaced with a single .flatMap() call.
Safe fix: Replace the chain with .flatMap().
(lint/complexity/useFlatMap)
270-295: Consider performance optimization for the LabelsGroup component.The component looks good, but consider memoizing it since it's used within a HoverCard that might trigger frequent re-renders.
-const LabelsGroup = ({ +const LabelsGroup = memo(({ 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> ); -}; +}); +LabelsGroup.displayName = "LabelsGroup";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/components/email-list/EmailListItem.tsx(4 hunks)apps/web/hooks/useLabels.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/web/components/email-list/EmailListItem.tsx
[error] 24-24: Some named imports are only used as types.
This import is only used as a type.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Add inline type keywords.
(lint/style/useImportType)
[error] 84-90: The call chain .map().flat() can be replaced with a single .flatMap() call.
Safe fix: Replace the chain with .flatMap().
(lint/complexity/useFlatMap)
🔇 Additional comments (2)
apps/web/hooks/useLabels.ts (1)
6-14: LGTM! Well-structured type definition for label colors.The
UserLabeltype is well-defined with clear color properties that enable complete customization of label appearance.apps/web/components/email-list/EmailListItem.tsx (1)
163-188: LGTM! Well-implemented label display with hover functionality.The implementation of label display with a "show more" hover card is clean and user-friendly. The use of
stopPropagationin the hover button prevents unintended email selection.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/web/components/email-list/EmailListItem.tsx (3)
24-24: Use type-only import forUserLabel.Since
UserLabelis only used as a type, it should be imported using thetypekeyword to ensure proper tree-shaking.-import { useLabels, UserLabel } from "@/hooks/useLabels"; +import { useLabels, type UserLabel } from "@/hooks/useLabels";🧰 Tools
🪛 Biome (1.9.4)
[error] 24-24: Some named imports are only used as types.
This import is only used as a type.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Add inline type keywords.(lint/style/useImportType)
83-92: Optimize array operations usingflatMap.The chain of
.map().flat()can be simplified using.flatMap()for better readability and performance.const threadLabels = useMemo(() => { - return thread.messages - .map((message) => - message.labelIds - ?.map((id) => userLabels?.find((label) => label.id === id)) - .filter(Boolean), - ) - .flat() - .filter(isDefined); + return thread.messages.flatMap((message) => + (message.labelIds ?? []) + .map((id) => userLabels?.find((label) => label.id === id)) + .filter(isDefined) + ); }, [thread.messages, userLabels]);🧰 Tools
🪛 Biome (1.9.4)
[error] 84-90: The call chain .map().flat() can be replaced with a single .flatMap() call.
Safe fix: Replace the chain with .flatMap().
(lint/complexity/useFlatMap)
270-295: Remove empty className prop from Badge component.The Badge component has an empty className prop which can be removed.
<Badge - className="" key={label.id} style={{ color: label?.color?.textColor, backgroundColor: label?.color?.backgroundColor, }} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/components/email-list/EmailListItem.tsx(4 hunks)apps/web/hooks/useLabels.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/hooks/useLabels.ts
🧰 Additional context used
🪛 Biome (1.9.4)
apps/web/components/email-list/EmailListItem.tsx
[error] 24-24: Some named imports are only used as types.
This import is only used as a type.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Add inline type keywords.
(lint/style/useImportType)
[error] 84-90: The call chain .map().flat() can be replaced with a single .flatMap() call.
Safe fix: Replace the chain with .flatMap().
(lint/complexity/useFlatMap)
🔇 Additional comments (1)
apps/web/components/email-list/EmailListItem.tsx (1)
163-188: LGTM! Good implementation of label display with hover functionality.The implementation effectively handles:
- Conditional rendering of labels
- "Show more" functionality using hover card
- Event propagation prevention
|
Thanks. Take a look at the nitpick comments from CodeRabbit. They'd be good to use here. @RicSala |
There was a problem hiding this comment.
make this whole thing a component. i think all the new code you added can be a component
There was a problem hiding this comment.
can email move it into its own file in the email-list folder
There was a problem hiding this comment.
possible we reuse this in other places in the app
There was a problem hiding this comment.
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.
I was on it! They are fixed and gone now! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/components/email-list/EmailListItem.tsx (2)
24-28: Consider movingMAX_THREAD_LABELSto a shared constants file.Since this constant might be reused in other components (e.g., in email details view), consider moving it to a shared constants file.
79-92: Extract label processing logic into a custom hook.Consider moving the label processing logic into a custom hook (e.g.,
useThreadLabels) to improve reusability and reduce component complexity. This would align with the component extraction suggestions from previous reviews.Example implementation:
// hooks/useThreadLabels.ts export function useThreadLabels(thread: Thread) { const { userLabels } = useLabels(); const threadLabels = useMemo(() => { return thread.messages .flatMap((message) => message.labelIds ?.map((id) => userLabels?.find((label) => label.id === id)) .filter(Boolean), ) .filter(isDefined); }, [thread.messages, userLabels]); return { threadLabels, hasLabels: threadLabels.length > 0, hasMoreLabels: threadLabels.length > MAX_THREAD_LABELS, }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/components/email-list/EmailListItem.tsx(3 hunks)
🔇 Additional comments (1)
apps/web/components/email-list/EmailListItem.tsx (1)
160-165: LGTM! Clean implementation of label display.The implementation correctly follows the component extraction pattern and handles the display of labels in a clean, maintainable way.
|
|
||
| const cta = findCtaLink(lastMessage.textHtml); | ||
|
|
||
| const { userLabels } = useLabels(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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:
-
Component Responsibility
- Even though SWR will deduplicate the actual API requests, the hook execution itself still runs for every row item
- The
EmailListItemcomponent should ideally be focused on presentation, not data fetching - Moving data fetching up reduces the cognitive load when reading the item component
-
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
- While SWR handles API deduplication, the hook still:
-
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.
There was a problem hiding this comment.
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!
|
We have a Gmail provider that I used for other pages just now: Can probably do the same thing for Mail page. The gmail provider just stores the state once for us so a little more efficient although probably minor difference. |
|
Thanks! This was merged in another PR! |
2 participants
Add coloured labels to the
EmailListItemcomponent in theapps/webdirectory.:apps/web/components/email-list/EmailListItem.tsx: ImporteduseLabels,UserLabel,Badge,MoreVertical,HoverCard, andisDefinedto support the new label functionality.apps/web/components/email-list/EmailListItem.tsx: Added logic to fetch and process user labels using theuseLabelshook, and computed label-related states (threadLabels,hasLabels,hasMoreLabels).apps/web/components/email-list/EmailListItem.tsx: Integrated theLabelsGroupcomponent to display labels within the email list items and added a hover card for additional labels.New component for label rendering:
apps/web/components/email-list/EmailListItem.tsx: Defined theLabelsGroupcomponent to handle the display of user labels with optional wrapping and styling based on label colors.Enhancements to user labels:
apps/web/hooks/useLabels.ts: Extended theUserLabeltype to include color properties (textColorandbackgroundColor).Summary by CodeRabbit