fixed #348 #396
fixed #348 #396
Conversation
|
@Saksham1387 is attempting to deploy a commit to the Inbox Zero Team on Vercel. A member of the Team first needs to authorize it. |
|
|
WalkthroughThe pull request enhances the Changes
Sequence Diagram(s)sequenceDiagram
participant E as EmailListItem
participant L as useLabels Hook
participant B as Badge Component
E->>L: Retrieve userLabels
Note over E: Memoize labelsToDisplay using labelIds & userLabels
E->>B: For each label in labelsToDisplay, render Badge
Assessment against linked issues
Possibly related PRs
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: 9.23.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by ✨ Finishing Touches
🪧 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)
156-168: Consider extracting duplicated label rendering codeThe label rendering code is duplicated in two locations. Consider extracting this into a reusable component or function to follow the DRY principle.
+ // Add this near the top of the file or in a separate component file + const EmailLabels = ({ labels }: { labels: Array<{ id: string, name: string }> }) => { + if (!labels || labels.length === 0) return null; + + return ( + <span className="ml-2 inline-flex flex-wrap items-center gap-1"> + {labels.map((label) => ( + <Badge + variant="secondary" + key={label.id} + className="text-xs" + > + {label.name} + </Badge> + ))} + </span> + ); + }; // Then replace the duplicated sections with: - {labelsToDisplay && labelsToDisplay.length > 0 && ( - <span className="ml-2 inline-flex flex-wrap items-center gap-1"> - {labelsToDisplay.map((label) => ( - <Badge - variant="secondary" - key={label.id} - className="text-xs" - > - {label.name} - </Badge> - ))} - </span> - )} + <EmailLabels labels={labelsToDisplay} />Also applies to: 226-238
63-65: Consider preserving label color informationIf the Gmail API provides color information for labels, consider incorporating it into the Badge component to match Gmail's visual representation.
- return { id, name: label.name }; + return { + id, + name: label.name, + color: label.color || undefined + }; // Then in the Badge component: <Badge variant="secondary" key={label.id} className="text-xs" + style={label.color ? { backgroundColor: label.color.backgroundColor, color: label.color.textColor } : undefined} > {label.name} </Badge>
61-67: Consider handling empty label arrays more efficientlyThe current implementation will return undefined if labelIds is undefined, but an empty array would be more consistent with the expected return type.
- return labelIds + return labelIds || []This ensures that when labelIds is undefined, an empty array is returned rather than undefined, which is more consistent with the expected array type and can prevent potential issues when checking length.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/components/email-list/EmailListItem.tsx(4 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
apps/web/components/email-list/EmailListItem.tsx (1)
apps/web/utils/types.ts (1)
isDefined(8-10)
🔇 Additional comments (3)
apps/web/components/email-list/EmailListItem.tsx (3)
24-26: Good additions to support label feature integrationThe new imports are well-chosen to support the label display functionality:
- Badge component for visual representation
- isDefined utility for safe filtering
- useLabels hook for accessing the label data
51-51: Appropriate hook usageThe useLabels hook is correctly used to retrieve user-defined labels.
59-68: Well-implemented memoization for label processingThis implementation correctly:
- Uses useMemo to optimize performance
- Handles the mapping of label IDs to their display names
- Filters out undefined values
- Sets proper dependencies to trigger recalculation when needed
| const labelIds = lastMessage?.labelIds; | ||
| return labelIds | ||
| ?.map((id) => { | ||
| const label = userLabels[Number(id)]; |
There was a problem hiding this comment.
otherwise it will be of type any and typescript complains
| {labelsToDisplay.map((label) => ( | ||
| <Badge | ||
| variant="secondary" | ||
| key={label.id} | ||
| className="text-xs" | ||
| > | ||
| {label.name} | ||
| </Badge> | ||
| ))} |
There was a problem hiding this comment.
this appears twice. we could make this a small component at the bottom of this file
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/components/email-list/EmailListItem.tsx(5 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
apps/web/components/email-list/EmailListItem.tsx (3)
apps/web/hooks/useLabels.ts (1)
useLabels(37-52)apps/web/utils/types.ts (1)
isDefined(8-10)apps/web/components/Badge.tsx (1)
Badge(30-47)
🔇 Additional comments (4)
apps/web/components/email-list/EmailListItem.tsx (4)
24-26: Appropriate imports added for label functionality.The new imports are well-chosen for implementing the label display feature. The
Badgecomponent will style the labels,isDefinedhelps with type-safe filtering, anduseLabelsprovides access to the user's labels.
51-51: Good use of the useLabels hook.The hook is correctly used to retrieve user labels for display in the component.
156-158: The label display logic appears in two locations.The labels are conditionally rendered in both the regular view and split view sections of the component. This duplication is necessary given the different UI layouts, and you've correctly extracted the rendering logic into a separate component to avoid code duplication.
Also applies to: 216-218
241-261: Well-structured label display component.The
LabelsDisplaycomponent is well-designed with proper type definitions, conditional rendering for empty states, and appropriate styling. The implementation correctly maps through the labels and displays them as badges.
| const labelsToDisplay = useMemo(() => { | ||
| const labelIds = lastMessage?.labelIds; | ||
| return labelIds | ||
| ?.map((id) => { | ||
| const label = userLabels[Number(id)]; | ||
| if (!label) return null; | ||
| return { id, name: label.name }; | ||
| }) | ||
| .filter(isDefined); | ||
| }, [lastMessage?.labelIds, userLabels]); |
There was a problem hiding this comment.
Fix the label lookup logic.
The current implementation treats userLabels as an object with numeric keys, but according to the useLabels hook implementation, it returns an array. You should use array methods like find to locate labels by ID.
const labelsToDisplay = useMemo(() => {
const labelIds = lastMessage?.labelIds;
return labelIds
?.map((id) => {
- const label = userLabels[Number(id)];
+ const label = userLabels.find(label => label.id === id);
if (!label) return null;
return { id, name: label.name };
})
.filter(isDefined);
}, [lastMessage?.labelIds, userLabels]);📝 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.
| const labelsToDisplay = useMemo(() => { | |
| const labelIds = lastMessage?.labelIds; | |
| return labelIds | |
| ?.map((id) => { | |
| const label = userLabels[Number(id)]; | |
| if (!label) return null; | |
| return { id, name: label.name }; | |
| }) | |
| .filter(isDefined); | |
| }, [lastMessage?.labelIds, userLabels]); | |
| const labelsToDisplay = useMemo(() => { | |
| const labelIds = lastMessage?.labelIds; | |
| return labelIds | |
| ?.map((id) => { | |
| const label = userLabels.find(label => label.id === id); | |
| if (!label) return null; | |
| return { id, name: label.name }; | |
| }) | |
| .filter(isDefined); | |
| }, [lastMessage?.labelIds, userLabels]); |
Fixes #348
Summary by CodeRabbit