Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning Rate limit exceeded@elie222 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis pull request adds a new development guidelines document ( Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant M as Mail Component
participant R as Threads Route
participant C as Threads Controller
participant V as Validation
participant API as Google Threads API
U->>M: Initiate request with searchParams (type, labelId)
M->>R: Send API request with query parameters
R->>R: Extract labelId and build query object
R->>C: Pass query to controller
C->>V: Validate query (including optional labelId)
V-->>C: Return validated query
C->>API: Fetch threads using labelId or type
API-->>C: Return threads data
C-->>M: Return threads data
M-->>U: Render threads
sequenceDiagram
participant U as User
participant S as SideNav
participant H as useLabels Hook
U->>S: Click toggle for hidden labels
S->>S: Update showHiddenLabels state
S->>H: Invoke useLabels({ includeHidden: true/false })
H->>H: Fetch and sort user labels into visible and hidden
H-->>S: Return visible and hidden labels
S-->>U: Update label navigation display
Poem
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 (5)
apps/web/app/(app)/mail/page.tsx (1)
18-18: Consider using zod for runtime type validation.The
searchParamstype could benefit from runtime validation to ensure type safety at the API boundary.+import { z } from "zod"; +const searchParamsSchema = z.object({ + type: z.string().optional(), + labelId: z.string().optional(), +}); export default function Mail({ searchParams, }: { - searchParams: { type?: string; labelId?: string }; + searchParams: z.infer<typeof searchParamsSchema>;apps/web/CLAUDE.md (2)
17-17: Fix Next.js spelling.The official spelling is "Next.js" rather than "NextJS".
-NextJS app router structure with (app) directory +Next.js app router structure with (app) directory🧰 Tools
🪛 LanguageTool
[uncategorized] ~17-~17: The official spelling of this programming framework is “Next.js”.
Context: ...se@/for imports from project root - NextJS app router structure with (app) directo...(NODE_JS)
27-27: Add missing articles.Add missing articles "a" and "the" for better readability.
-Ensure responsive design with mobile-first approach +Ensure responsive design with a mobile-first approach -Use LoadingContent component for async data: +Use the LoadingContent component for async data:Also applies to: 30-30
🧰 Tools
🪛 LanguageTool
[uncategorized] ~27-~27: You might be missing the article “a” here.
Context: ...ailable - Ensure responsive design with mobile-first approach - Follow consistent nami...(AI_EN_LECTOR_MISSING_DETERMINER_A)
apps/web/components/SideNav.tsx (1)
338-349: Enhance button accessibility.The toggle button for hidden labels should have an aria-label and aria-expanded attribute.
<button type="button" onClick={() => setShowHiddenLabels(!showHiddenLabels)} + aria-label="Toggle hidden labels" + aria-expanded={showHiddenLabels} className="flex w-full items-center px-3 py-2 text-sm text-muted-foreground hover:bg-accent hover:text-accent-foreground" >apps/web/hooks/useLabels.ts (1)
6-12: Consider making visibility properties more type-safe.The visibility properties could use string literals for better type safety.
export type UserLabel = { id: string; name: string; type: "user"; - labelListVisibility?: string; - messageListVisibility?: string; + labelListVisibility?: "labelShow" | "labelHide" | "labelShowIfUnread"; + messageListVisibility?: "show" | "hide"; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/web/CLAUDE.md(1 hunks)apps/web/app/(app)/mail/page.tsx(1 hunks)apps/web/app/api/google/threads/controller.ts(1 hunks)apps/web/app/api/google/threads/route.ts(1 hunks)apps/web/app/api/google/threads/validation.ts(1 hunks)apps/web/components/SideNav.tsx(5 hunks)apps/web/components/SideNavMenu.tsx(2 hunks)apps/web/hooks/useLabels.ts(1 hunks)
🧰 Additional context used
🪛 LanguageTool
apps/web/CLAUDE.md
[uncategorized] ~17-~17: The official spelling of this programming framework is “Next.js”.
Context: ...se @/ for imports from project root - NextJS app router structure with (app) directo...
(NODE_JS)
[uncategorized] ~27-~27: You might be missing the article “a” here.
Context: ...ailable - Ensure responsive design with mobile-first approach - Follow consistent nami...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~30-~30: You might be missing the article “the” here.
Context: ... dedicated type files when shared - Use LoadingContent component for async data: ```tsx <L...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🔇 Additional comments (10)
apps/web/components/SideNavMenu.tsx (2)
18-18: LGTM! The newactiveproperty enhances control over navigation states.The optional
activeproperty provides explicit control over a nav item's active state, independent of URL matching.
34-34: LGTM! The active state logic is correctly implemented.The
isActiveprop now considers both the explicitactiveproperty and URL matching, providing flexible control over navigation highlighting.apps/web/app/api/google/threads/validation.ts (1)
9-9: LGTM! The schema correctly defines the optional label ID property.The
labelIdproperty is properly defined as a nullable string, maintaining consistency with other query parameters and Gmail's API requirements.apps/web/app/api/google/threads/controller.ts (1)
47-47: LGTM! The label handling logic is well-implemented.The code correctly prioritizes explicit label IDs while maintaining backward compatibility with type-based label filtering.
apps/web/app/api/google/threads/route.ts (2)
17-17: LGTM! The label ID parameter is correctly extracted.The code properly extracts the optional label ID from search parameters.
24-24: LGTM! The label ID is properly included in query validation.The extracted label ID is correctly passed to the validation schema.
apps/web/app/(app)/mail/page.tsx (1)
20-27: LGTM! Clean query parameter handling.The query construction logic is well-organized and handles the parameters appropriately.
apps/web/CLAUDE.md (1)
1-41: LGTM! Comprehensive development guidelines.The guidelines provide clear instructions for development practices, component usage, and environment configuration.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~17-~17: The official spelling of this programming framework is “Next.js”.
Context: ...se@/for imports from project root - NextJS app router structure with (app) directo...(NODE_JS)
[uncategorized] ~27-~27: You might be missing the article “a” here.
Context: ...ailable - Ensure responsive design with mobile-first approach - Follow consistent nami...(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~30-~30: You might be missing the article “the” here.
Context: ... dedicated type files when shared - Use LoadingContent component for async data: ```tsx <L...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
apps/web/components/SideNav.tsx (1)
267-269: LGTM! Well-structured label management.The implementation of label management is clean and efficient:
- Good use of useMemo for performance optimization
- Clear separation of visible and hidden labels
- Proper URL parameter handling
Also applies to: 272-283, 286-297
apps/web/hooks/useLabels.ts (1)
51-60: LGTM! Efficient label sorting implementation.The sorting logic is well-implemented:
- Handles edge cases with null/undefined names
- Properly orders bracketed labels
- Uses localeCompare for correct string comparison
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/web/hooks/useLabels.ts (2)
7-13: Add JSDoc documentation for the UserLabel type.Consider adding documentation to explain the purpose of
labelListVisibilityandmessageListVisibilityproperties, their possible values, and their impact on label visibility.+/** + * Represents a user-created Gmail label. + * @property labelListVisibility - Controls label visibility in the label list (e.g., 'labelShow', 'labelHide') + * @property messageListVisibility - Controls label visibility in the message list + */ export type UserLabel = { id: string; name: string; type: "user"; labelListVisibility?: string; messageListVisibility?: string; };
48-57: Add null checks in sortLabels function.While the function handles empty strings, it should also handle null or undefined values to prevent potential runtime errors.
function sortLabels(a: UserLabel, b: UserLabel) { - const aName = a.name || ""; - const bName = b.name || ""; + const aName = a?.name ?? ""; + const bName = b?.name ?? ""; // Order words that start with [ at the end if (aName.startsWith("[") && !bName.startsWith("[")) return 1; if (!aName.startsWith("[") && bName.startsWith("[")) return -1; return aName.localeCompare(bName); }apps/web/components/SideNav.tsx (1)
272-283: Optimize label transformation performance.Consider moving the URL parsing logic outside the memoized function to prevent unnecessary re-parsing on each render.
+const getSearchParams = (path: string) => new URLSearchParams(path.split("?")[1] || ""); const labelNavItems = useMemo(() => { - const searchParams = new URLSearchParams(path.split("?")[1] || ""); + const searchParams = getSearchParams(path); const currentLabelId = searchParams.get("labelId"); return userLabels.map((label) => ({ name: label.name, icon: TagIcon, href: `?type=label&labelId=${encodeURIComponent(label.id)}`, active: currentLabelId === label.id, })); }, [userLabels, path]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/components/SideNav.tsx(5 hunks)apps/web/hooks/useLabels.ts(1 hunks)
🔇 Additional comments (1)
apps/web/components/SideNav.tsx (1)
12-13: LGTM!The new imports are appropriate for the added functionality.
Also applies to: 51-51
apps/web/hooks/useLabels.ts
Outdated
| const { userLabels, hiddenUserLabels } = useMemo(() => { | ||
| // Always get visible labels | ||
| const visible = allUserLabels.filter(isHiddenLabel).sort(sortLabels); | ||
|
|
||
| // Only process hidden labels if needed | ||
| const hidden = options.includeHidden | ||
| ? allUserLabels.filter(isHiddenLabel).sort(sortLabels) | ||
| : []; | ||
|
|
||
| return { userLabels: visible, hiddenUserLabels: hidden }; | ||
| }, [allUserLabels, options.includeHidden]); |
There was a problem hiding this comment.
Fix incorrect label filtering logic.
The current implementation has a bug where isHiddenLabel is used for both visible and hidden labels, which will result in incorrect filtering. The visible labels should use the opposite condition.
const { userLabels, hiddenUserLabels } = useMemo(() => {
// Always get visible labels
- const visible = allUserLabels.filter(isHiddenLabel).sort(sortLabels);
+ const visible = allUserLabels.filter((label) => !isHiddenLabel(label)).sort(sortLabels);
// Only process hidden labels if needed
const hidden = options.includeHidden
? allUserLabels.filter(isHiddenLabel).sort(sortLabels)
: [];
return { userLabels: visible, hiddenUserLabels: hidden };
}, [allUserLabels, options.includeHidden]);📝 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 { userLabels, hiddenUserLabels } = useMemo(() => { | |
| // Always get visible labels | |
| const visible = allUserLabels.filter(isHiddenLabel).sort(sortLabels); | |
| // Only process hidden labels if needed | |
| const hidden = options.includeHidden | |
| ? allUserLabels.filter(isHiddenLabel).sort(sortLabels) | |
| : []; | |
| return { userLabels: visible, hiddenUserLabels: hidden }; | |
| }, [allUserLabels, options.includeHidden]); | |
| const { userLabels, hiddenUserLabels } = useMemo(() => { | |
| // Always get visible labels | |
| const visible = allUserLabels.filter((label) => !isHiddenLabel(label)).sort(sortLabels); | |
| // Only process hidden labels if needed | |
| const hidden = options.includeHidden | |
| ? allUserLabels.filter(isHiddenLabel).sort(sortLabels) | |
| : []; | |
| return { userLabels: visible, hiddenUserLabels: hidden }; | |
| }, [allUserLabels, options.includeHidden]); |
| <LoadingContent loading={isLoading}> | ||
| {userLabels.length > 0 ? ( | ||
| <SideNavMenu items={labelNavItems} activeHref={path} /> | ||
| ) : ( | ||
| <div className="px-3 py-2 text-xs text-muted-foreground"> | ||
| No labels | ||
| </div> | ||
| )} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the loading state.
The LoadingContent component should handle error states from the useLabels hook.
-<LoadingContent loading={isLoading}>
+<LoadingContent loading={isLoading} error={error}>
{userLabels.length > 0 ? (
<SideNavMenu items={labelNavItems} activeHref={path} />
) : (
<div className="px-3 py-2 text-xs text-muted-foreground">
- No labels
+ {error ? 'Failed to load labels' : 'No labels'}
</div>
)}📝 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.
| <LoadingContent loading={isLoading}> | |
| {userLabels.length > 0 ? ( | |
| <SideNavMenu items={labelNavItems} activeHref={path} /> | |
| ) : ( | |
| <div className="px-3 py-2 text-xs text-muted-foreground"> | |
| No labels | |
| </div> | |
| )} | |
| <LoadingContent loading={isLoading} error={error}> | |
| {userLabels.length > 0 ? ( | |
| <SideNavMenu items={labelNavItems} activeHref={path} /> | |
| ) : ( | |
| <div className="px-3 py-2 text-xs text-muted-foreground"> | |
| {error ? 'Failed to load labels' : 'No labels'} | |
| </div> | |
| )} |
| <button | ||
| type="button" | ||
| onClick={() => setShowHiddenLabels(!showHiddenLabels)} | ||
| className="flex w-full items-center px-3 py-2 text-sm text-muted-foreground hover:bg-accent hover:text-accent-foreground" | ||
| > | ||
| {showHiddenLabels ? ( | ||
| <ChevronDownIcon className="mr-1 size-4" /> | ||
| ) : ( | ||
| <ChevronRightIcon className="mr-1 size-4" /> | ||
| )} | ||
| <span>More</span> | ||
| </button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility attributes to the toggle button.
The toggle button for hidden labels should have proper accessibility attributes for better screen reader support.
<button
type="button"
onClick={() => setShowHiddenLabels(!showHiddenLabels)}
+ aria-expanded={showHiddenLabels}
+ aria-controls="hidden-labels-menu"
className="flex w-full items-center px-3 py-2 text-sm text-muted-foreground hover:bg-accent hover:text-accent-foreground"
>📝 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.
| <button | |
| type="button" | |
| onClick={() => setShowHiddenLabels(!showHiddenLabels)} | |
| className="flex w-full items-center px-3 py-2 text-sm text-muted-foreground hover:bg-accent hover:text-accent-foreground" | |
| > | |
| {showHiddenLabels ? ( | |
| <ChevronDownIcon className="mr-1 size-4" /> | |
| ) : ( | |
| <ChevronRightIcon className="mr-1 size-4" /> | |
| )} | |
| <span>More</span> | |
| </button> | |
| <button | |
| type="button" | |
| onClick={() => setShowHiddenLabels(!showHiddenLabels)} | |
| aria-expanded={showHiddenLabels} | |
| aria-controls="hidden-labels-menu" | |
| className="flex w-full items-center px-3 py-2 text-sm text-muted-foreground hover:bg-accent hover:text-accent-foreground" | |
| > | |
| {showHiddenLabels ? ( | |
| <ChevronDownIcon className="mr-1 size-4" /> | |
| ) : ( | |
| <ChevronRightIcon className="mr-1 size-4" /> | |
| )} | |
| <span>More</span> | |
| </button> |
|
Closes: #347 |
1 participant
Summary by CodeRabbit
New Features
Improvements
useLabelshook now supports an option to include hidden labels, enhancing label management capabilities.