[Web] Refactor/sidebar #3361
[Web] Refactor/sidebar #3361
Conversation
WalkthroughThe pull request introduces multiple changes across various files, primarily focusing on enhancing user interface components, adding new environment variables for Mailchimp integration, and updating the spell-checking dictionary. Key modifications include the addition of new words to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MailchimpAPI
participant App
User->>App: Subscribe to newsletter
App->>User: Request email and captcha
User->>App: Provide email and captcha
App->>MailchimpAPI: Send subscription request
MailchimpAPI-->>App: Response (success/error)
App->>User: Notify subscription status
Possibly related PRs
Suggested labels
Suggested reviewers
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 (1.23.1)
apps/web/app/[locale]/layout.tsxOops! Something went wrong! :( ESLint: 8.46.0 ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct. The config "next/core-web-vitals" was referenced from the config file in "/apps/web/.eslintrc.json". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. 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 (
|
NdekoCode
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🧹 Outside diff range and nitpick comments (34)
apps/web/components/ui/label.tsx (1)
9-9: Consider adding size and appearance variants for better flexibility.While the base styling is good, the component could be more reusable with additional variants. Consider adding size variants (sm, md, lg) and appearance variants (default, error) to support different use cases.
-const labelVariants = cva('text-sm font-medium leading-none peer-disabled:cursor-not-allowed peer-disabled:opacity-70'); +const labelVariants = cva( + 'font-medium leading-none peer-disabled:cursor-not-allowed peer-disabled:opacity-70', + { + variants: { + size: { + sm: 'text-xs', + md: 'text-sm', + lg: 'text-base' + }, + variant: { + default: 'text-gray-900', + error: 'text-red-500' + } + }, + defaultVariants: { + size: 'md', + variant: 'default' + } + } +);apps/web/components/ui/card.tsx (3)
5-8: Consider using semantic HTML for better accessibility.While the implementation is solid, consider using
<article>or<section>instead of<div>for better semantic meaning, especially if the card represents a self-contained composition.-const Card = React.forwardRef<HTMLDivElement, React.HTMLAttributes<HTMLDivElement>>(({ className, ...props }, ref) => ( - <div ref={ref} className={cn('rounded-lg border bg-card text-card-foreground shadow-sm', className)} {...props} /> +const Card = React.forwardRef<HTMLElement, React.HTMLAttributes<HTMLElement>>(({ className, ...props }, ref) => ( + <article ref={ref} className={cn('rounded-lg border bg-card text-card-foreground shadow-sm', className)} {...props} /> ));
10-41: Consider enhancing type safety and component organization.The implementation is solid, but could benefit from the following improvements:
- Define specific prop interfaces for each component
- Consider composing components using the Compound Component pattern
Example implementation:
interface CardProps extends React.HTMLAttributes<HTMLElement> { variant?: 'default' | 'bordered'; } interface CardHeaderProps extends React.HTMLAttributes<HTMLDivElement> { // Add specific props if needed } // Similar interfaces for other components const Card = React.forwardRef<HTMLElement, CardProps>(({ variant = 'default', className, ...props }, ref) => { // Implementation }) as CardComponent; type CardComponent = React.ForwardRefExoticComponent<CardProps> & { Header: typeof CardHeader; Title: typeof CardTitle; // ... other components }; // Attach subcomponents Card.Header = CardHeader; Card.Title = CardTitle; // ... other attachmentsThis would enable a more intuitive usage:
<Card> <Card.Header> <Card.Title>Title</Card.Title> </Card.Header> </Card>
1-43: Consider adding component documentation and examples.Since this is a new reusable UI component, it would be valuable to:
- Add JSDoc comments describing each component's purpose and props
- Create Storybook stories or examples showcasing different use cases
- Consider adding aria-* attributes for better accessibility
Example JSDoc:
/** * Card component that serves as a container for related content. * * @example * <Card> * <CardHeader> * <CardTitle>Title</CardTitle> * <CardDescription>Description</CardDescription> * </CardHeader> * <CardContent>Content</CardContent> * </Card> */apps/web/.env.sample (2)
90-93: Consider grouping Mailchimp configuration with other external services.The Mailchimp configuration would be better organized if placed with other third-party service integrations (like OAuth, SMTP) rather than at the end of the file.
MEET_JWT_APP_SECRET= + +# Mailchimp +MAILCHIMP_API_KEY= +MAILCHIMP_LIST_ID= # Warning: IF TRUE This allows production builds to successfully complete even if # your project has ESLint errors. NEXT_IGNORE_ESLINT_ERROR_ON_BUILD=true - -# Mailchimp -MAILCHIMP_API_KEY= -MAILCHIMP_LIST_ID=
91-93: Add documentation for Mailchimp credentials format and security.The Mailchimp configuration lacks important documentation about:
- The expected format of the API key
- Where to obtain these credentials
- Security implications of these sensitive values
# Mailchimp -MAILCHIMP_API_KEY= -MAILCHIMP_LIST_ID= +# Get these from your Mailchimp account settings +# API Key format: <dc>-us<number> +MAILCHIMP_API_KEY= # Keep this secure and never commit the actual value +MAILCHIMP_LIST_ID= # Also known as Audience ID in Mailchimp dashboardapps/web/components/pages/task/description-block/editor-components/LinkElement.tsx (2)
Line range hint
38-73: Enhance accessibility for the popup menu.The popup menu lacks proper ARIA attributes and keyboard navigation support.
Consider the following improvements:
- Add ARIA attributes to the popup
- Implement keyboard navigation
- Add a visible focus state
<div className="absolute left-0 flex items-center max-w-xs bg-white dark:bg-dark--theme-light p-2 gap-1 rounded-md border border-gray-300 dark:border-[#7B8089] z-10 " contentEditable="false" + role="menu" + aria-label="Link options" > <div className="flex items-center pr-2 text-xs border-r dark:bg-dark--theme-light border-gray-300"> <ExternalLinkIcon className="h-[18px] w-[18px] mr-1" /> <a href={href} rel="noreferrer" target="_blank" className="text-[#5000B9] dark:text-primary-light truncate max-w-[230px] overflow-hidden whitespace-nowrap mr-0" style={{ textOverflow: 'ellipsis' }} + role="menuitem" > {element.href} </a> </div> <button onClick={() => removeLink(editor)} className="border-none bg-transparent hover:cursor-pointer ml-0 " + role="menuitem" + aria-label="Remove link" > <UnlinkRoundIcon className="h-4 w-4 text-black dark:text-white" /> </button> </div>
Line range hint
33-35: Consider sanitizing URLs before display and navigation.The URL manipulation could be improved to handle malicious inputs more safely.
-const href = `https://${element?.href?.replace('https://', '')}`; +const sanitizeUrl = (url: string): string => { + try { + const urlObj = new URL(url.startsWith('http') ? url : `https://${url}`); + return urlObj.toString(); + } catch { + return '#'; + } +}; +const href = sanitizeUrl(element?.href || '');apps/web/components/pages/main/header-tabs.tsx (6)
11-11: Consider improving prop names and adding documentation.The prop names could be more descriptive:
linkAlldoesn't clearly indicate its purposekanbancould be renamed to something more specific likeisKanbanView-const HeaderTabs = ({ linkAll, kanban = false }: { linkAll: boolean; kanban?: boolean }) => { +interface HeaderTabsProps { + /** Whether to enable routing for all tab views */ + enableRouting: boolean; + /** Whether the component is being used in Kanban view */ + isKanbanView?: boolean; +} + +const HeaderTabs = ({ enableRouting, isKanbanView = false }: HeaderTabsProps) => {
13-18: Move options array outside component.The options array is static and should be defined outside the component to prevent unnecessary recreation on each render.
+const VIEW_OPTIONS = [ + { label: 'CARDS', icon: QueueListIcon, view: IssuesView.CARDS }, + { label: 'TABLE', icon: TableCellsIcon, view: IssuesView.TABLE }, + { label: 'BLOCKS', icon: Squares2X2Icon, view: IssuesView.BLOCKS }, + { label: 'KANBAN', icon: KanbanIcon, view: IssuesView.KANBAN } +] as const; + const HeaderTabs = ({ linkAll, kanban = false }) => { const t = useTranslations(); - const options = [...]
19-19: Improve links array clarity and maintainability.The links array uses magic strings and its relationship with the options array isn't immediately clear.
-const links = linkAll ? ['/', '/', '/', '/kanban'] : [undefined, undefined, undefined, '/kanban']; +const ROUTE_PATHS = { + DEFAULT: '/', + KANBAN: '/kanban' +} as const; + +const links = VIEW_OPTIONS.map(option => + linkAll + ? option.view === IssuesView.KANBAN ? ROUTE_PATHS.KANBAN : ROUTE_PATHS.DEFAULT + : option.view === IssuesView.KANBAN ? ROUTE_PATHS.KANBAN : undefined +);
29-29: Document the purpose of z-20 class.The z-index value seems arbitrary. Consider documenting why this specific z-index is needed and if it follows your application's z-index system.
Add a comment explaining the z-index choice:
className="z-20" // Ensures tooltip appears above other UI elements in the header
38-42: Simplify click handler logic.The click handler condition can be simplified and made more explicit.
-onClick={() => { - if (links[index] !== '/kanban') { - setView(optionView); - } -}} +onClick={() => { + const isKanbanView = optionView === IssuesView.KANBAN; + if (!isKanbanView) { + setView(optionView); + } +}}
33-37: Consider using a more maintainable approach for conditional classes.The conditional classes could be more readable using a dedicated styling object or CSS modules.
+const getTabStyles = (isActive: boolean) => ({ + button: clsxm( + 'rounded-md p-2 text-gray-700 dark:text-gray-300', + isActive && 'bg-gray-100 text-gray-900 dark:bg-gray-800 dark:text-gray-100' + ), + icon: clsxm( + 'w-5 h-5 inline text-gray-600 dark:text-gray-400', + isActive && 'dark:text-white text-gray-800' + ) +}); -className={clsxm( - 'rounded-md p-2 text-gray-700 dark:text-gray-300', - activeView === optionView && - 'bg-gray-100 text-gray-900 dark:bg-gray-800 dark:text-gray-100' -)} +className={getTabStyles(activeView === optionView).button} -className={clsxm( - 'w-5 h-5 inline text-gray-600 dark:text-gray-400', - activeView === optionView && 'dark:text-white text-gray-800' -)} +className={getTabStyles(activeView === optionView).icon}Also applies to: 45-48
apps/web/components/workspace-switcher.tsx (2)
16-25: LGTM! Consider enhancing type safety.The component declaration and props structure look good. The use of
Readonlyfor props is a good practice for immutability.Consider extracting the workspace type to a separate interface for better reusability:
interface Workspace { name: string; logo: React.ElementType; plan: string; } export function WorkspacesSwitcher({ workspaces }: Readonly<{ workspaces: Workspace[]; }>)
Line range hint
67-73: Add proper handling for workspace creation.The "Add team" button appears to be non-functional and still references teams instead of workspaces.
Consider implementing proper workspace creation handling:
-<DropdownMenuItem className="gap-2 p-2"> +<DropdownMenuItem + className="gap-2 p-2" + onClick={() => handleAddWorkspace()} + role="menuitem" + aria-label="Create new workspace" +> <div className="flex items-center justify-center border rounded-md size-6 bg-background"> <Plus className="size-4" /> </div> - <div className="font-medium text-muted-foreground">Add team</div> + <div className="font-medium text-muted-foreground">Add workspace</div> </DropdownMenuItem>Would you like me to help implement the
handleAddWorkspacefunction?apps/web/components/nav-projects.tsx (3)
27-41: Consider keeping theReadonlytype wrapper for better type safety.While removing
Readonlysimplifies the type, keeping it helps prevent accidental mutations of the props object, which is a React best practice.-}: { +}: Readonly<{ projects: { name: string; url: string; icon: LucideIcon; }[]; -}) { +}>) {
46-46: Use optional chaining for better null safety.The condition can be simplified using optional chaining.
-{projects && projects.length ? ( +{projects?.length ? (🧰 Tools
🪛 Biome (1.9.4)
[error] 46-46: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
96-96: Extract hardcoded styles to a CSS class.The button styles contain hardcoded values which should be moved to a CSS class for better maintainability.
-className="w-full text-xs mt-3 dark:text-white rounded-xl border-[0.0938rem] w-full max-w-[230px]" +className="create-project-button"Add to your CSS:
.create-project-button { @apply w-full text-xs mt-3 dark:text-white rounded-xl border max-w-[230px]; border-width: 0.0938rem; }apps/web/components/sidebar-opt-in-form.tsx (1)
16-20: Consider enhancing email validation rules.The current email validation is basic. Consider adding more specific validation rules for better user experience.
const subscribeFormSchema = z .object({ - email: z.string().email() + email: z.string() + .email('Please enter a valid email address') + .min(5, 'Email must be at least 5 characters') + .max(255, 'Email must not exceed 255 characters') + .refine((email) => !email.endsWith('.test'), { + message: 'Test email domains are not allowed' + }) }) .required();apps/web/app/[locale]/page-component.tsx (2)
Line range hint
42-57: Consider optimizing localStorage effectThe useEffect for fullWidth mode could be optimized to avoid multiple localStorage reads and unnecessary re-renders.
Consider this improvement:
React.useEffect(() => { - window && window?.localStorage.getItem('conf-fullWidth-mode'); - setFullWidth(JSON.parse(window?.localStorage.getItem('conf-fullWidth-mode') || 'true')); + const storedValue = window?.localStorage.getItem('conf-fullWidth-mode'); + if (storedValue !== null) { + setFullWidth(JSON.parse(storedValue)); + } }, [setFullWidth]);
Line range hint
108-127: Simplify mobile responsiveness in TaskTimerSectionThe current implementation uses multiple conditional classes and a separate mobile toggle, which could be simplified for better maintainability.
Consider using a more declarative approach:
- <AuthUserTaskInput - className={clsxm( - 'w-full lg:basis-3/4 grow', - !showInput && '!hidden md:!flex', - !isTrackingEnabled && 'md:w-full' - )} - /> - <div - onClick={() => setShowInput((p) => !p)} - className="border dark:border-[#26272C] w-full rounded p-2 md:hidden flex justify-center mt-2" - > - <ChevronDown className={clsxm('h-12 transition-all', showInput && 'rotate-180')}> - {showInput ? 'hide the issue input' : 'show the issue input'} - </ChevronDown> - </div> + <div className="w-full"> + <AuthUserTaskInput + className={clsxm( + 'w-full transition-all duration-200', + 'lg:basis-3/4 grow', + 'md:flex', + !isTrackingEnabled && 'md:w-full', + { + 'max-h-0 md:max-h-full overflow-hidden': !showInput, + 'max-h-[200px]': showInput + } + )} + /> + <button + onClick={() => setShowInput((p) => !p)} + className="w-full flex justify-center items-center p-2 md:hidden border dark:border-[#26272C] rounded mt-2" + aria-label={showInput ? 'Hide issue input' : 'Show issue input'} + > + <ChevronDown className={clsxm('h-12 transition-transform duration-200', showInput && 'rotate-180')} /> + </button> + </div>This approach:
- Uses CSS transitions for smoother animations
- Improves accessibility with proper button semantics
- Simplifies the class structure
- Uses height transitions instead of display properties
apps/web/package.json (1)
91-91: Consider pinning the geist versionWhile using
^1.3.1allows for minor version updates, considering this is a UI framework being used in a sidebar refactor, it might be safer to pin to an exact version to prevent unexpected visual changes.- "geist": "^1.3.1", + "geist": "1.3.1",apps/web/components/nav-main.tsx (2)
68-68: Consider i18n for the Platform labelThe hardcoded "Platform" text should be internationalized for better maintainability and multi-language support.
- <SidebarGroupLabel>Platform</SidebarGroupLabel> + <SidebarGroupLabel>{t('common.platform')}</SidebarGroupLabel>
Line range hint
149-174: Consolidate styling and improve accessibilityThe sub-menu styling has some good improvements but needs attention in a few areas:
- The ChevronRight icon should have a more descriptive aria-label
- The width constraint is duplicated from the parent component
Consider these improvements:
- <ChevronRight /> - <span className="sr-only">Toggle</span> + <ChevronRight aria-label={`Toggle ${item.title} sub-menu`} /> - <SidebarMenuSubItem className="w-full max-w-[230px]" key={key}> + <SidebarMenuSubItem className="w-full" key={key}>apps/web/styles/globals.css (2)
Line range hint
279-281: Consider using Tailwind's built-in tracking utilities.The
.details-labelclass uses a custom tracking value (-0.02em). Consider using Tailwind's built-in tracking utilities (tracking-tighter, tracking-tight, etc.) for better maintainability and consistency.-.details-label { - @apply not-italic font-medium text-[0.625rem] leading-[140%] tracking-[-0.02em] text-[#A5A2B2] 2xl:text-xs; -} +.details-label { + @apply not-italic font-medium text-[0.625rem] leading-[140%] tracking-tight text-[#A5A2B2] 2xl:text-xs; +}
Line range hint
285-299: Improve search input styling maintainability.Several issues with the search input styling could be improved:
- Fixed heights might cause layout issues with different font sizes
- Use of
!importantin placeholder styles- Commented code should be removed
Consider this more maintainable approach:
.search-border { - height: 33px; + @apply h-[33px]; } .search-border .input-border { - border-width: 0px !important; - height: 40px; + @apply border-0 h-10; } .search-border .input-border input { - height: 40px; + @apply h-10; } .search-border .input-border input::placeholder { - color: #7e7991; - font-size: 15px !important; - /* font-weight: 500; */ - letter-spacing: 0.8px; + @apply text-[#7e7991] text-[15px] tracking-[0.8px]; }apps/web/components/ui/sidebar.tsx (3)
Line range hint
67-76: Enhance cookie security settings.The cookie implementation should include additional security flags.
-document.cookie = `${SIDEBAR_COOKIE_NAME}=${nextOpen}; path=/; max-age=${SIDEBAR_COOKIE_MAX_AGE}`; +document.cookie = `${SIDEBAR_COOKIE_NAME}=${nextOpen}; path=/; max-age=${SIDEBAR_COOKIE_MAX_AGE}; SameSite=Lax; Secure`;
Line range hint
84-94: Improve keyboard shortcut accessibility.The keyboard shortcut implementation should include ARIA labels and consider users who might not be able to use modifier keys.
const handleKeyDown = (event: KeyboardEvent) => { if (event.key === SIDEBAR_KEYBOARD_SHORTCUT && (event.metaKey || event.ctrlKey)) { event.preventDefault(); toggleSidebar(); + // Announce state change for screen readers + const message = state === 'expanded' ? 'Sidebar collapsed' : 'Sidebar expanded'; + document.getElementById('a11y-status')?.setAttribute('aria-label', message); } };Consider adding this element to your main layout:
<div id="a11y-status" role="status" aria-live="polite" className="sr-only" />
Line range hint
183-183: Improve z-index management.Using z-[9999] is an anti-pattern that can lead to layering issues. Consider implementing a z-index scale system.
Create a new constants file for z-index management:
// constants/z-index.ts export const ZINDEX = { sidebar: 50, modal: 100, tooltip: 150, // ... other values } as const;Then update the z-index values:
-className="w-[--sidebar-width] bg-sidebar p-0 text-sidebar-foreground [&>button]:hidden z-[9999]" +className="w-[--sidebar-width] bg-sidebar p-0 text-sidebar-foreground [&>button]:hidden z-[var(--z-sidebar)]"Also applies to: 249-249
apps/web/components/app-sidebar.tsx (4)
8-11: Avoid potential naming conflicts withXimportImporting
Xdirectly might cause conflicts if there's an existing component or variable namedXin your codebase. Consider aliasing it to a more descriptive name to prevent potential naming collisions.Apply this diff to alias the
Ximport:import { MonitorSmartphone, LayoutDashboard, Heart, SquareActivity, Files, - X, + X as CloseIcon, Command, AudioWaveform, GalleryVerticalEnd } from 'lucide-react';And update its usage accordingly:
-<X +<CloseIcon className="w-5 h-5 cursor-pointer" onClick={() => toggleFavorite(task)} />
45-95: Replace hardcoded sample data with dynamic dataThe
workspacesarray currently contains hardcoded sample data. For production, consider fetching workspace data dynamically from an API or using state management to ensure the sidebar displays actual user data.
48-77: Extract inline SVG logo into a separate componentThe inline SVG for the 'Ever Teams' logo increases code complexity and reduces readability. Consider extracting it into a separate component or importing it from an external file to improve maintainability.
Line range hint
121-174: Use unique identifiers for keys in list rendering instead of indexUsing the array index
indexas thekeyprop in list rendering can lead to issues when the list changes. It's better to use a unique and stable identifier liketask.id.Apply this diff to use
task.idas the key:.map((task, index) => ({ title: task?.title, url: '#', component: ( <SidebarMenuSubButton - key={index} + key={task.id} className={cn( 'hover:bg-[#eaeef4] first:mt-1 last:mb-1 flex items-center text-[#1F2937] dark:text-gray-50 data-[active=true]:bg-[#eaeef4] min-h-10 h-10 dark:hover:bg-sidebar-accent transition-colors duration-300' )} asChild >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (28)
.cspell.json(6 hunks)apps/web/.env(1 hunks)apps/web/.env.sample(1 hunks)apps/web/app/[locale]/layout.tsx(1 hunks)apps/web/app/[locale]/page-component.tsx(1 hunks)apps/web/app/api/subscribe/route.ts(1 hunks)apps/web/components/app-sidebar.tsx(4 hunks)apps/web/components/nav-main.tsx(8 hunks)apps/web/components/nav-projects.tsx(2 hunks)apps/web/components/nav-secondary.tsx(1 hunks)apps/web/components/pages/all-teams/header-tabs.tsx(1 hunks)apps/web/components/pages/main/header-tabs.tsx(1 hunks)apps/web/components/pages/task/description-block/editor-components/LinkElement.tsx(1 hunks)apps/web/components/sidebar-opt-in-form.tsx(1 hunks)apps/web/components/ui/card.tsx(1 hunks)apps/web/components/ui/form.tsx(1 hunks)apps/web/components/ui/label.tsx(1 hunks)apps/web/components/ui/sidebar.tsx(1 hunks)apps/web/components/workspace-switcher.tsx(4 hunks)apps/web/lib/features/task/daily-plan/views-header-tabs.tsx(1 hunks)apps/web/lib/features/task/task-input-kanban.tsx(2 hunks)apps/web/lib/features/task/task-input.tsx(6 hunks)apps/web/lib/features/user-nav-menu.tsx(2 hunks)apps/web/lib/layout/main-layout.tsx(1 hunks)apps/web/next.config.js(1 hunks)apps/web/package.json(5 hunks)apps/web/styles/globals.css(1 hunks)apps/web/tailwind.config.js(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- apps/web/app/[locale]/layout.tsx
- apps/web/components/pages/all-teams/header-tabs.tsx
- apps/web/lib/features/task/daily-plan/views-header-tabs.tsx
- apps/web/lib/features/user-nav-menu.tsx
- apps/web/lib/layout/main-layout.tsx
- apps/web/tailwind.config.js
🧰 Additional context used
🪛 Biome (1.9.4)
apps/web/components/nav-projects.tsx
[error] 46-46: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/web/lib/features/task/task-input-kanban.tsx
[error] 83-83: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 138-138: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 157-158: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 170-170: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 195-195: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 259-259: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/web/lib/features/task/task-input.tsx
[error] 117-117: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 122-122: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 173-173: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 204-204: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 224-225: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 260-260: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 329-329: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
[error] 347-347: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 376-376: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 400-400: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 633-633: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 647-647: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (23)
apps/web/components/ui/label.tsx (3)
1-8: LGTM! Well-structured imports with proper client directive.
The imports are well-organized and the 'use client' directive is correctly placed. Using Radix UI primitives is a good choice for building accessible form components.
19-19: LGTM! Clean named export.
The named export is appropriate for this component and follows React best practices.
1-19: Verify integration with FormLabel component.
The AI summary mentions integration with a FormLabel component. Let's verify this integration to ensure proper form handling capabilities.
✅ Verification successful
Let's examine the form integration by checking the content of these files, particularly form.tsx which likely contains the FormLabel component.
Let's check the actual usage of FormLabel in the task-row component to complete our verification.
Label component is properly integrated with form handling
The verification shows that:
- The Label component is correctly imported and used in the form system
- FormLabel is implemented as a wrapper around the Label component in
form.tsx - It properly handles form context, error states, and accessibility attributes (htmlFor, aria-describedby)
- The integration provides proper form handling capabilities including error states and accessibility features
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for FormLabel component and its usage of this Label component
rg -l "FormLabel|import.*Label.*from.*label" apps/web/components/
Length of output: 203
Script:
#!/bin/bash
# Check the content of form.tsx to understand the Label integration
rg -A 10 "FormLabel|import.*Label.*from.*label" apps/web/components/ui/form.tsx
Length of output: 1448
Script:
#!/bin/bash
# Check how FormLabel is used in the task-row component
rg -A 5 "FormLabel|import.*Label.*from.*label" apps/web/components/pages/task/details-section/components/task-row.tsx
Length of output: 252
apps/web/components/nav-secondary.tsx (2)
30-31: LGTM! Width constraints improve layout consistency
The addition of w-full max-w-[230px] to both the MenuItem and MenuButton ensures:
- Consistent width across all menu items
- Proper button sizing within the sidebar
- Prevention of unwanted stretching
28-28: Verify the reduced gap spacing visually
The vertical gap between menu items has been reduced from gap-y-3 (0.75rem) to gap-y-1 (0.25rem). While this change helps with space efficiency, please ensure it maintains good visual hierarchy and readability.
✅ Verification successful
The gap-y-1 spacing is consistent with other sidebar components
The reduced gap spacing from gap-y-3 to gap-y-1 aligns with the spacing used in other sidebar components:
nav-main.tsxusesgap-y-1for its main menu itemsnav-secondary.tsx(the file in review) usesgap-y-1- Other sidebar components like
nav-projects.tsxuse consistent width constraints
This change maintains visual consistency across the application's navigation components, making it a valid improvement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of gap-y-1 or gap-y-3 in sidebar components
# to verify consistency across the application
rg --type tsx "gap-y-[13]" "apps/web/components"
Length of output: 76
Script:
#!/bin/bash
# Search for gap-y patterns in all files under components directory
rg "gap-y-[13]" apps/web/components/
Length of output: 641
Script:
#!/bin/bash
# Search for SidebarMenu component usage to check spacing patterns
rg "SidebarMenu" apps/web/components/
Length of output: 7159
Script:
#!/bin/bash
# Find all tsx/jsx files in components directory that might contain sidebar-related code
fd -e tsx -e jsx . apps/web/components/ --exec grep -l "sidebar"
Length of output: 578
apps/web/components/ui/card.tsx (2)
1-4: LGTM! Clean and minimal imports.
The imports are well-organized and include only the necessary dependencies.
43-43: LGTM! Clean exports.
The named exports approach is perfect for tree-shaking and explicit imports.
apps/web/.env.sample (2)
91-93: Verify Mailchimp configuration completeness.
Please confirm if additional Mailchimp configuration variables are needed for:
- Server Region/Data Center
- Audience/List Name (for logging/debugging)
- Default subscription status
✅ Verification successful
The Mailchimp configuration is complete and well-implemented
The current implementation already handles all necessary Mailchimp configuration aspects:
- Server Region/Data Center is automatically extracted from the API key format (
<key>-us3) - Default subscription status is hardcoded as 'subscribed'
- Default tag 'Ever Teams' is set if no tags provided
- Error handling and validation are properly implemented
No additional environment variables are needed as the implementation follows Mailchimp's best practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if any Mailchimp-related code references additional environment variables
rg -i "process\.env\.MAILCHIMP" --type ts
Length of output: 250
Script:
#!/bin/bash
# Check the implementation details of the Mailchimp subscription route
cat apps/web/app/api/subscribe/route.ts
Length of output: 2123
92-93: Verify no actual credentials are committed.
Ensure that these empty values don't contain any actual Mailchimp credentials, even in the commit history.
✅ Verification successful
No Mailchimp credentials exposed in environment files
The environment files have been checked and contain no exposed Mailchimp credentials. The .env.sample file correctly shows empty placeholder values, and no Mailchimp-related credentials were found in other environment files or related utilities. The only API-like string found was an Expo project ID, which is not a security concern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential API keys in the commit history
rg -i "mailchimp.*[a-f0-9]{32}"
rg -i "list-id.*[a-f0-9]{10}"
Length of output: 65
Script:
#!/bin/bash
# Let's check the .env.sample file directly and also look for any Mailchimp-related patterns
cat apps/web/.env.sample | grep -i mailchimp
# Also check for any potential API key patterns in the file
rg -i "[a-f0-9-]{32,}" apps/web/.env.sample
# Let's also check other env files that might exist
fd -t f ".env"
Length of output: 229
Script:
#!/bin/bash
# Let's check the other env files for any potential Mailchimp credentials
cat apps/mobile/.env.template | grep -i mailchimp
# Let's also check the content of the check-provider-env-vars file for any hardcoded values
cat apps/web/app/utils/check-provider-env-vars.ts | grep -i mailchimp
# Additional check for any potential API key patterns in these files
rg -i "[a-f0-9-]{32,}" apps/mobile/.env.template apps/web/app/utils/check-provider-env-vars.ts
Length of output: 299
apps/web/components/pages/task/description-block/editor-components/LinkElement.tsx (1)
56-56: LGTM on the max-width adjustment.
The reduction in max-width from 240px to 230px appears reasonable for maintaining consistent layout within the sidebar context.
apps/web/components/workspace-switcher.tsx (1)
Line range hint 16-77: Verify workspace switching integration.
Since this is a significant refactor from teams to workspaces, we should verify the integration points.
✅ Verification successful
Workspace switching integration looks correct
The verification shows that:
- The
WorkspacesSwitchercomponent is properly integrated inapp-sidebar.tsxand receives the requiredworkspacesprop fromdata.workspaces - No legacy
TeamSwitcherreferences remain in the codebase - The component is correctly placed within the
SidebarHeadersection of the sidebar layout
The refactoring from teams to workspaces appears to be consistent in this component's integration points.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining team references and verify workspace integration
# Check for any remaining team references in the codebase
echo "Checking for remaining team references..."
rg -l "TeamSwitcher" apps/
# Check for workspace switcher usage
echo "Checking workspace switcher usage..."
rg -l "WorkspacesSwitcher" apps/
# Check for potential breaking changes in parent components
echo "Checking sidebar integration..."
rg -l "SidebarMenu.*WorkspacesSwitcher" apps/
Length of output: 435
Script:
#!/bin/bash
# Let's examine the integration in app-sidebar.tsx and check the props being passed
echo "Checking WorkspacesSwitcher usage in app-sidebar.tsx..."
rg -A 10 "WorkspacesSwitcher" apps/web/components/app-sidebar.tsx
# Let's also check for any team-related props or state management
echo "Checking for team/workspace state management..."
rg -l "useTeam|useWorkspace" apps/web/components/
# Check for any workspace-related types
echo "Checking workspace types..."
ast-grep --pattern 'type $_ = {
$$$
workspace$_: $_
$$$
}'
Length of output: 2482
apps/web/.env (1)
141-143: LGTM! Mailchimp configuration variables added correctly.
The new environment variables for Mailchimp integration follow the established pattern in the file, with clear section header and empty default values.
apps/web/components/nav-projects.tsx (1)
Line range hint 1-25: LGTM! Imports and setup are well-organized.
The imports are properly structured and all dependencies are necessary for the component's functionality.
🧰 Tools
🪛 Biome (1.9.4)
[error] 46-46: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/web/components/sidebar-opt-in-form.tsx (1)
1-11: LGTM! Imports are well-organized and necessary.
All required dependencies are properly imported for form handling, validation, and UI components.
apps/web/app/[locale]/page-component.tsx (1)
69-71: Verify header behavior during scrolling
The removal of the sticky behavior from the header might affect the user experience during scrolling, especially on longer pages. Consider if this was intentional as part of the sidebar refactoring.
Let's check if there are any other sticky headers in the codebase that might conflict:
If the header should remain visible while scrolling, consider one of these approaches:
- Restore the sticky behavior with proper z-index management
- Implement a collapsible header that shows/hides based on scroll direction
✅ Verification successful
Header sticky behavior removal is intentional
The codebase search reveals that sticky headers are used selectively in specific components like calendar setup, dropdowns, and team views, but not in the main page layout. This suggests the removal of sticky behavior from the header in page-component.tsx is intentional, aligning with the sidebar refactoring changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for sticky header implementations
rg -g '*.{tsx,jsx,css,scss}' -i '(position:\s*sticky|sticky\s+top|className=.*sticky)'
Length of output: 588
apps/web/package.json (2)
40-40: Form handling dependencies look well-structured
The combination of react-hook-form, @hookform/resolvers, and zod is a robust choice for form handling and validation in the sidebar component. This setup provides:
- Type-safe form validation with Zod schemas
- Efficient form state management with react-hook-form
- Integration between the two via @hookform/resolvers
Let's verify these dependencies are being used together correctly:
Also applies to: 121-121, 137-137
✅ Verification successful
Form validation setup is correctly implemented in the sidebar component
The codebase shows proper integration of the form handling dependencies in sidebar-opt-in-form.tsx:
- Uses
zodResolverfrom@hookform/resolvers/zodfor schema validation - Implements
useFormwith the resolver configuration - Includes proper schema validation with Zod
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for form validation setup patterns
rg -A 5 "useForm|zodResolver|z\.object"
Length of output: 20238
40-40: Verify peer dependency compatibility
Please ensure these new dependencies have compatible peer dependencies with the existing packages in the project.
Also applies to: 91-91, 121-121, 137-137
✅ Verification successful
Peer dependencies are compatible
The package @hookform/resolvers is being used with react-hook-form@^7.53.2 which is the correct version range for compatibility. No peer dependency conflicts were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential peer dependency conflicts
yarn why @hookform/resolvers
yarn why geist
yarn why zod
Length of output: 2306
Script:
#!/bin/bash
# Let's try a different approach to check package dependencies
cat apps/web/package.json | jq '.dependencies + .devDependencies'
# Also check if there are any peer dependency warnings in package-lock.json or yarn.lock
fd -t f "package-lock.json|yarn.lock" -x cat {}
Length of output: 135722
Script:
#!/bin/bash
# Let's check the package.json for peer dependencies of @hookform/resolvers
cat apps/web/package.json | jq '.peerDependencies'
# Also check if react-hook-form is installed since it's commonly a peer dependency
cat apps/web/package.json | jq '.dependencies["react-hook-form"]'
Length of output: 133
apps/web/components/nav-main.tsx (2)
8-8: LGTM: Clean import addition
The SidebarGroupLabel import is properly integrated with other sidebar-related imports.
Line range hint 1-195: Verify collapsed state behavior and keyboard navigation
Please ensure:
- The collapsed state properly maintains accessibility
- Keyboard navigation works correctly through the menu hierarchy
- Screen readers can properly announce the menu structure
✅ Verification successful
Keyboard navigation and accessibility features are properly implemented
The implementation has robust accessibility features:
- The sidebar uses Radix UI's primitives (Collapsible, Tooltip) which provide built-in accessibility
- Proper ARIA attributes and roles through Radix UI components
- Keyboard shortcut (Ctrl/Cmd + B) for toggling sidebar
- Screen reader support with proper labeling (e.g., "Toggle Sidebar" in sr-only spans)
- Focus management through outline and ring styling
- Proper tab navigation structure through semantic HTML (nav, button, link elements)
- Tooltip support for collapsed state to show labels
- Proper state management for expanded/collapsed views
The collapsed state maintains accessibility through:
- Visible icons with tooltips
- Keyboard shortcut support
- Screen reader announcements
- Focus indicators
- Semantic HTML structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other accessibility-related components and keyboard handlers
rg -l "useKeyboard|aria-|role=" --type tsx
# Look for related test files
fd -e test.tsx -e spec.tsx nav-main
Length of output: 110
Script:
#!/bin/bash
# Check for accessibility attributes and keyboard handlers with proper file extension
rg -l "useKeyboard|aria-|role=" --type ts --type tsx --type jsx --type js
# Look for test files with various extensions
fd "nav-main" -e test.tsx -e test.ts -e test.jsx -e test.js -e spec.tsx -e spec.ts -e spec.jsx -e spec.js
# Check for keyboard event handlers in the nav component
rg "onKeyDown|onKeyPress|onKeyUp" apps/web/components/nav-main.tsx
# Check for aria attributes in the nav component
rg "aria-|role=" apps/web/components/nav-main.tsx
Length of output: 330
Script:
#!/bin/bash
# Let's check the sidebar component since it's imported and used extensively
cat apps/web/components/ui/sidebar.tsx
# Check the collapsible component for accessibility features
cat apps/web/components/ui/collapsible.tsx
# Look for any accessibility tests in the components directory
find apps/web/components -type f -name "*test*" -o -name "*spec*"
Length of output: 23306
.cspell.json (1)
9-9: LGTM! Verify word usage in codebase.
The new dictionary entries align well with the PR's objectives of sidebar refactoring and Mailchimp integration. The words follow proper naming conventions.
Let's verify that these new words are actually used in the codebase:
Also applies to: 28-28, 97-97, 246-246, 373-373, 407-407
✅ Verification successful
All new dictionary words are actively used in the codebase
All the newly added words to .cspell.json are verified to be in use:
- "hookform": Used in imports from "@hookform/resolvers"
- "apikey": Used in API route files for authentication
- "DATACENTER": Used in Mailchimp API integration
- "mailchimp": Used extensively in subscribe route implementation
- "timesheet-viewMode": Used in timesheet page component
- "Waitlist": Used in sidebar opt-in form component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the newly added words are used in the codebase
# Note: Using case-insensitive search as specified in the config
echo "Checking usage of new dictionary words..."
# Search for each word
for word in hookform apikey DATACENTER mailchimp "timesheet-viewMode" Waitlist; do
echo "Searching for: $word"
rg -i "$word" --type-not json
done
Length of output: 2976
apps/web/styles/globals.css (1)
152-152: Verify text readability across different viewport sizes.
The addition of text-sm utility class to the body element will ensure consistent text sizing across the application.
Let's check if this change affects any existing responsive text utilities:
✅ Verification successful
The text-sm utility is safely applied and doesn't conflict with other text size utilities
The text size utilities in the codebase are properly scoped:
- Body text uses
text-smas a default size - Tooltip uses
text-xsspecifically for its content - Details label has its own responsive sizing (
text-[0.625rem]totext-xs) - Nivo calendar text is specifically scoped with
text-sm
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for responsive text size utilities that might conflict with text-sm
rg -l 'text-(xs|sm|base|lg|xl)' --type css
Length of output: 156
Script:
#!/bin/bash
# Check the context of text size utilities in globals.css to understand potential conflicts
rg 'text-(xs|sm|base|lg|xl)' apps/web/styles/globals.css -B 2 -A 2
Length of output: 545
apps/web/components/ui/sidebar.tsx (2)
Line range hint 391-673: LGTM! Well-structured component hierarchy.
The supporting components (SidebarMenu*, SidebarGroup*, etc.) demonstrate:
- Good use of TypeScript
- Proper composition patterns
- Consistent prop interfaces
- Clear separation of concerns
16-18: Verify the impact of sidebar width changes on existing layouts.
The sidebar widths have been reduced:
- Full width: 17rem → 16rem
- Icon width: 5rem → 3rem
These changes could affect existing layouts and content alignment.
✅ Verification successful
Based on the search results, I can now provide a final response:
The sidebar width changes appear safe to implement
The width changes (17rem → 16rem and 5rem → 3rem) are localized to the sidebar component and properly handled through CSS variables that are consistently used throughout the codebase. The search results show:
- The sidebar widths are defined as CSS variables (
--sidebar-width,--sidebar-width-icon) and referenced appropriately in all dependent components - No hardcoded references to the old width values (17rem or 5rem) were found in layouts or other components
- The responsive layout system properly handles width transitions through CSS variables
The changes reduce the sidebar width slightly while maintaining the existing responsive behavior and layout structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any hardcoded width values in the codebase that might need adjustment
# Look for any CSS/style references to the old widths
rg -t css -t tsx -t ts "17rem|5rem"
# Look for potential layout dependencies
rg -t tsx -t ts "sidebar.*width|width.*sidebar"
Length of output: 143
Script:
#!/bin/bash
# Search for any references to the old widths using ripgrep without file type filters
rg "17rem|5rem"
# Look for potential layout dependencies
rg "sidebar.*width|width.*sidebar"
# Search for any style-related files that might contain width references
fd "\.(css|scss|tsx|ts|js|jsx)$" --exec rg -l "rem"
Length of output: 59143
| const Label = React.forwardRef< | ||
| React.ElementRef<typeof LabelPrimitive.Root>, | ||
| React.ComponentPropsWithoutRef<typeof LabelPrimitive.Root> & VariantProps<typeof labelVariants> | ||
| >(({ className, ...props }, ref) => ( | ||
| <LabelPrimitive.Root ref={ref} className={cn(labelVariants(), className)} {...props} /> | ||
| )); | ||
| Label.displayName = LabelPrimitive.Root.displayName; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding required prop validation for accessibility.
While the implementation is solid, consider adding validation for the htmlFor prop to ensure proper label-input association, which is crucial for accessibility.
const Label = React.forwardRef<
React.ElementRef<typeof LabelPrimitive.Root>,
React.ComponentPropsWithoutRef<typeof LabelPrimitive.Root> & VariantProps<typeof labelVariants>
->(({ className, ...props }, ref) => (
+>(({ className, htmlFor, ...props }, ref) => {
+ if (process.env.NODE_ENV !== 'production' && !htmlFor) {
+ console.warn(
+ 'Label component: Missing htmlFor prop. This may cause accessibility issues.'
+ );
+ }
+ return (
<LabelPrimitive.Root ref={ref} className={cn(labelVariants(), className)} {...props} />
-));
+ );
+});📝 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 Label = React.forwardRef< | |
| React.ElementRef<typeof LabelPrimitive.Root>, | |
| React.ComponentPropsWithoutRef<typeof LabelPrimitive.Root> & VariantProps<typeof labelVariants> | |
| >(({ className, ...props }, ref) => ( | |
| <LabelPrimitive.Root ref={ref} className={cn(labelVariants(), className)} {...props} /> | |
| )); | |
| Label.displayName = LabelPrimitive.Root.displayName; | |
| const Label = React.forwardRef< | |
| React.ElementRef<typeof LabelPrimitive.Root>, | |
| React.ComponentPropsWithoutRef<typeof LabelPrimitive.Root> & VariantProps<typeof labelVariants> | |
| >(({ className, htmlFor, ...props }, ref) => { | |
| if (process.env.NODE_ENV !== 'production' && !htmlFor) { | |
| console.warn( | |
| 'Label component: Missing htmlFor prop. This may cause accessibility issues.' | |
| ); | |
| } | |
| return ( | |
| <LabelPrimitive.Root ref={ref} className={cn(labelVariants(), className)} {...props} /> | |
| ); | |
| }); | |
| Label.displayName = LabelPrimitive.Root.displayName; |
| className=" text-[#5000B9] dark:text-primary-light truncate max-w-[230px] overflow-hidden whitespace-nowrap mr-0" | ||
| style={{ textOverflow: 'ellipsis' }} | ||
| > | ||
| {element.href} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing security for external links.
The link element could benefit from additional security attributes when opening external URLs.
<a
href={href}
rel="noreferrer"
target="_blank"
className=" text-[#5000B9] dark:text-primary-light truncate max-w-[230px] overflow-hidden whitespace-nowrap mr-0"
+ rel="noopener noreferrer"
style={{ textOverflow: 'ellipsis' }}
>Committable suggestion skipped: line range outside the PR's diff.
| export const POST = async (req: NextRequest) => { | ||
| // 1. Destructure the email address from the request body. | ||
| const reqData = (await req.json()) as { | ||
| email_address: string; | ||
| tags: string[]; | ||
| captcha?: string; | ||
| }; | ||
|
|
||
| if (!reqData.email_address) { | ||
| // 2. Throw an error if an email wasn't provided. | ||
| return NextResponse.json({ error: 'Email is required' }, { status: 400 }); | ||
| } | ||
|
|
||
| if (!reqData.captcha) { | ||
| // 2. Display an error if the captcha code wasn't provided. | ||
| console.error('ERROR: Please provide required fields', 'STATUS: 400'); | ||
| } | ||
|
|
There was a problem hiding this comment.
Strengthen input validation and security measures
The current input validation has several security concerns:
- Email validation is too permissive
- Captcha validation is ineffective (logs but doesn't block)
- Missing rate limiting could enable API abuse
Consider implementing these improvements:
export const POST = async (req: NextRequest) => {
+ // Rate limiting
+ const ip = req.headers.get('x-forwarded-for') || 'unknown';
+ const rateLimiter = await getRateLimiter(ip);
+ if (!rateLimiter.isAllowed) {
+ return NextResponse.json(
+ { error: 'Too many requests' },
+ { status: 429 }
+ );
+ }
+
const reqData = (await req.json()) as {
email_address: string;
tags: string[];
captcha?: string;
};
- if (!reqData.email_address) {
+ const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
+ if (!reqData.email_address || !emailRegex.test(reqData.email_address)) {
return NextResponse.json({ error: 'Email is required' }, { status: 400 });
}
if (!reqData.captcha) {
- console.error('ERROR: Please provide required fields', 'STATUS: 400');
+ return NextResponse.json(
+ { error: 'Captcha verification failed' },
+ { status: 400 }
+ );
}
+
+ // Verify captcha
+ const isCaptchaValid = await verifyCaptcha(reqData.captcha);
+ if (!isCaptchaValid) {
+ return NextResponse.json(
+ { error: 'Invalid captcha' },
+ { status: 400 }
+ );
+ }Committable suggestion skipped: line range outside the PR's diff.
| try { | ||
| // 3. Fetch the environment variables. | ||
| const LIST_ID = process.env.MAILCHIMP_LIST_ID; | ||
| const API_KEY = process.env.MAILCHIMP_API_KEY ? process.env.MAILCHIMP_API_KEY : ''; | ||
| if (!LIST_ID || !API_KEY) { | ||
| throw new Error('Missing Mailchimp environment variables'); | ||
| } | ||
| // 4. API keys are in the form <key>-us3. | ||
| const DATACENTER = API_KEY.split('-')[1]; |
There was a problem hiding this comment.
Improve API key validation and error handling
The current API key handling is fragile and could fail silently with malformed keys.
Apply this improvement:
try {
const LIST_ID = process.env.MAILCHIMP_LIST_ID;
- const API_KEY = process.env.MAILCHIMP_API_KEY ? process.env.MAILCHIMP_API_KEY : '';
+ const API_KEY = process.env.MAILCHIMP_API_KEY?.trim() || '';
+
if (!LIST_ID || !API_KEY) {
throw new Error('Missing Mailchimp environment variables');
}
- const DATACENTER = API_KEY.split('-')[1];
+
+ // Validate API key format
+ const API_KEY_REGEX = /^[a-f0-9]{32}-([a-z]{2}\d{1,2})$/;
+ const match = API_KEY.match(API_KEY_REGEX);
+ if (!match) {
+ throw new Error('Invalid Mailchimp API key format');
+ }
+ const DATACENTER = match[1];📝 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.
| try { | |
| // 3. Fetch the environment variables. | |
| const LIST_ID = process.env.MAILCHIMP_LIST_ID; | |
| const API_KEY = process.env.MAILCHIMP_API_KEY ? process.env.MAILCHIMP_API_KEY : ''; | |
| if (!LIST_ID || !API_KEY) { | |
| throw new Error('Missing Mailchimp environment variables'); | |
| } | |
| // 4. API keys are in the form <key>-us3. | |
| const DATACENTER = API_KEY.split('-')[1]; | |
| try { | |
| // 3. Fetch the environment variables. | |
| const LIST_ID = process.env.MAILCHIMP_LIST_ID; | |
| const API_KEY = process.env.MAILCHIMP_API_KEY?.trim() || ''; | |
| if (!LIST_ID || !API_KEY) { | |
| throw new Error('Missing Mailchimp environment variables'); | |
| } | |
| // Validate API key format | |
| const API_KEY_REGEX = /^[a-f0-9]{32}-([a-z]{2}\d{1,2})$/; | |
| const match = API_KEY.match(API_KEY_REGEX); | |
| if (!match) { | |
| throw new Error('Invalid Mailchimp API key format'); | |
| } | |
| const DATACENTER = match[1]; |
| const mailchimpData = { | ||
| email_address: reqData.email_address, | ||
| status: 'subscribed', | ||
| tags: reqData.tags ? [...reqData.tags] : ['Ever Teams'] | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance data preparation and configuration flexibility
The current implementation has hardcoded values and lacks validation for tags.
Consider these improvements:
+ const DEFAULT_TAG = process.env.MAILCHIMP_DEFAULT_TAG || 'Ever Teams';
+
+ // Validate tags
+ const validateTags = (tags: string[]) => {
+ return tags.every(tag =>
+ typeof tag === 'string' &&
+ tag.length <= 100 &&
+ /^[\w\s-]+$/.test(tag)
+ );
+ };
+
const mailchimpData = {
email_address: reqData.email_address,
status: 'subscribed',
- tags: reqData.tags ? [...reqData.tags] : ['Ever Teams']
+ tags: reqData.tags && validateTags(reqData.tags)
+ ? [...reqData.tags]
+ : [DEFAULT_TAG]
};📝 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 mailchimpData = { | |
| email_address: reqData.email_address, | |
| status: 'subscribed', | |
| tags: reqData.tags ? [...reqData.tags] : ['Ever Teams'] | |
| }; | |
| const DEFAULT_TAG = process.env.MAILCHIMP_DEFAULT_TAG || 'Ever Teams'; | |
| // Validate tags | |
| const validateTags = (tags: string[]) => { | |
| return tags.every(tag => | |
| typeof tag === 'string' && | |
| tag.length <= 100 && | |
| /^[\w\s-]+$/.test(tag) | |
| ); | |
| }; | |
| const mailchimpData = { | |
| email_address: reqData.email_address, | |
| status: 'subscribed', | |
| tags: reqData.tags && validateTags(reqData.tags) | |
| ? [...reqData.tags] | |
| : [DEFAULT_TAG] | |
| }; |
| return state == 'expanded' ? ( | ||
| <Form {...form}> | ||
| <Card className="p-1 mt-auto shadow-none"> | ||
| <form onSubmit={form.handleSubmit(onSubmit)}> | ||
| <CardHeader className="flex flex-col gap-1 p-4 pb-0"> | ||
| <CardTitle className="text-sm font-medium">Subscribe to our newsletter</CardTitle> | ||
| <CardDescription className="text-xs"> | ||
| Opt-in to receive updates and news about Ever Teams. | ||
| </CardDescription> | ||
| </CardHeader> | ||
| <CardContent className="grid gap-2.5 p-4"> | ||
| <FormField | ||
| control={form.control} | ||
| name="email" | ||
| render={({ field }) => ( | ||
| <FormItem> | ||
| <FormControl> | ||
| <SidebarInput | ||
| type="email" | ||
| placeholder="Email" | ||
| className="placeholder:text-xs" | ||
| {...field} | ||
| /> | ||
| </FormControl> | ||
| <FormMessage className="text-xs" /> | ||
| </FormItem> | ||
| )} | ||
| /> | ||
| <Button | ||
| type="submit" | ||
| className="w-full shadow-none bg-sidebar-primary text-sidebar-primary-foreground" | ||
| size="sm" | ||
| > | ||
| {isLoading ? 'Subscribing...' : 'Subscribe'} | ||
| </Button> | ||
| </CardContent> | ||
| </form> | ||
| </Card> | ||
| </Form> | ||
| ) : null; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and user experience.
The form could benefit from improved accessibility and user feedback during loading states.
<Card className="p-1 mt-auto shadow-none">
- <form onSubmit={form.handleSubmit(onSubmit)}>
+ <form
+ onSubmit={form.handleSubmit(onSubmit)}
+ aria-label="Newsletter subscription form"
+ >
<CardHeader className="flex flex-col gap-1 p-4 pb-0">
<CardTitle className="text-sm font-medium">Subscribe to our newsletter</CardTitle>
<CardDescription className="text-xs">
Opt-in to receive updates and news about Ever Teams.
</CardDescription>
</CardHeader>
<CardContent className="grid gap-2.5 p-4">
<FormField
control={form.control}
name="email"
render={({ field }) => (
<FormItem>
<FormControl>
<SidebarInput
type="email"
placeholder="Email"
className="placeholder:text-xs"
+ aria-label="Email address"
+ disabled={isLoading}
+ autoComplete="email"
{...field}
/>
</FormControl>
<FormMessage className="text-xs" />
</FormItem>
)}
/>
<Button
type="submit"
className="w-full shadow-none bg-sidebar-primary text-sidebar-primary-foreground"
size="sm"
+ disabled={isLoading}
+ aria-busy={isLoading}
>
{isLoading ? 'Subscribing...' : 'Subscribe'}
</Button>
</CardContent>
</form>
</Card>📝 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.
| return state == 'expanded' ? ( | |
| <Form {...form}> | |
| <Card className="p-1 mt-auto shadow-none"> | |
| <form onSubmit={form.handleSubmit(onSubmit)}> | |
| <CardHeader className="flex flex-col gap-1 p-4 pb-0"> | |
| <CardTitle className="text-sm font-medium">Subscribe to our newsletter</CardTitle> | |
| <CardDescription className="text-xs"> | |
| Opt-in to receive updates and news about Ever Teams. | |
| </CardDescription> | |
| </CardHeader> | |
| <CardContent className="grid gap-2.5 p-4"> | |
| <FormField | |
| control={form.control} | |
| name="email" | |
| render={({ field }) => ( | |
| <FormItem> | |
| <FormControl> | |
| <SidebarInput | |
| type="email" | |
| placeholder="Email" | |
| className="placeholder:text-xs" | |
| {...field} | |
| /> | |
| </FormControl> | |
| <FormMessage className="text-xs" /> | |
| </FormItem> | |
| )} | |
| /> | |
| <Button | |
| type="submit" | |
| className="w-full shadow-none bg-sidebar-primary text-sidebar-primary-foreground" | |
| size="sm" | |
| > | |
| {isLoading ? 'Subscribing...' : 'Subscribe'} | |
| </Button> | |
| </CardContent> | |
| </form> | |
| </Card> | |
| </Form> | |
| ) : null; | |
| return state == 'expanded' ? ( | |
| <Form {...form}> | |
| <Card className="p-1 mt-auto shadow-none"> | |
| <form | |
| onSubmit={form.handleSubmit(onSubmit)} | |
| aria-label="Newsletter subscription form" | |
| > | |
| <CardHeader className="flex flex-col gap-1 p-4 pb-0"> | |
| <CardTitle className="text-sm font-medium">Subscribe to our newsletter</CardTitle> | |
| <CardDescription className="text-xs"> | |
| Opt-in to receive updates and news about Ever Teams. | |
| </CardDescription> | |
| </CardHeader> | |
| <CardContent className="grid gap-2.5 p-4"> | |
| <FormField | |
| control={form.control} | |
| name="email" | |
| render={({ field }) => ( | |
| <FormItem> | |
| <FormControl> | |
| <SidebarInput | |
| type="email" | |
| placeholder="Email" | |
| className="placeholder:text-xs" | |
| aria-label="Email address" | |
| disabled={isLoading} | |
| autoComplete="email" | |
| {...field} | |
| /> | |
| </FormControl> | |
| <FormMessage className="text-xs" /> | |
| </FormItem> | |
| )} | |
| /> | |
| <Button | |
| type="submit" | |
| className="w-full shadow-none bg-sidebar-primary text-sidebar-primary-foreground" | |
| size="sm" | |
| disabled={isLoading} | |
| aria-busy={isLoading} | |
| > | |
| {isLoading ? 'Subscribing...' : 'Subscribe'} | |
| </Button> | |
| </CardContent> | |
| </form> | |
| </Card> | |
| </Form> | |
| ) : null; |
| 'hover:bg-[#eaeef4] text-[#1F2937] items-center dark:text-gray-50 data-[active=true]:bg-[#eaeef4] min-h-10 h-10 dark:hover:bg-sidebar-accent px-3 py-2 transition-colors duration-300 !text-sm', | ||
| state === 'collapsed' ? ' justify-center' : '', | ||
| index === activeMenuIndex | ||
| ? 'font-medium bg-[#eaeef4] dark:text-gray-50 dark:bg-sidebar-accent' | ||
| : '!font-normal' // Style for active menu | ||
| ? 'font-normal bg-[#eaeef4] dark:text-gray-50 dark:bg-sidebar-accent' | ||
| : '!font-light' // Style for active menu |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repeated styling patterns and use theme variables
Several styling issues need attention:
- Duplicate styling logic between menu and sub-menu buttons
- Hardcoded color values (#eaeef4, #1F2937)
- Overuse of !important flags for font weights
Consider creating a shared style utility:
const getMenuButtonStyles = (isCollapsed: boolean, isActive: boolean) => cn(
'hover:bg-accent text-foreground items-center dark:text-gray-50',
'data-[active=true]:bg-accent min-h-10 h-10',
'dark:hover:bg-sidebar-accent px-3 py-2',
'transition-colors duration-300 text-sm',
isCollapsed && 'justify-center',
isActive ? 'font-normal bg-accent dark:bg-sidebar-accent' : 'font-light'
);Also applies to: 117-121
| <SidebarMenu | ||
| className={cn('w-full max-w-[230px]', state === 'collapsed' ? 'items-center gap-y-2' : '', 'gap-y-1')} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating width constraints and gap handling
- The max-width constraint (230px) is duplicated in child components (see line 157)
- There are potentially conflicting gap classes:
gap-y-1andgap-y-2for collapsed state
Consider moving the width constraint to a CSS variable and using a single gap strategy:
- className={cn('w-full max-w-[230px]', state === 'collapsed' ? 'items-center gap-y-2' : '', 'gap-y-1')}
+ className={cn('w-full var(--sidebar-width)', state === 'collapsed' ? 'items-center' : '', 'gap-y-2')}Committable suggestion skipped: line range outside the PR's diff.
| @@ -44,6 +44,7 @@ const eslintBuildConfig = process.env.NEXT_IGNORE_ESLINT_ERROR_ON_BUILD | |||
| const nextConfig = { | |||
| output: ['standalone', 'export'].includes(BUILD_OUTPUT_MODE) ? BUILD_OUTPUT_MODE : undefined, | |||
| reactStrictMode: false, | |||
| transpilePackages: ['geist'], | |||
There was a problem hiding this comment.
💡 Codebase verification
Geist package setup needs attention
The verification reveals potential issues with the geist package setup:
- While geist v1.3.1 is correctly listed in package.json dependencies and properly configured in next.config.js for transpilation, there are no direct imports or usage of geist components found in the codebase.
- The codebase appears to use custom components (Button, Input, Card, Modal) instead of geist components, suggesting the geist dependency might be unnecessary.
Recommendations:
- Remove the unused geist dependency and its transpilation configuration if it's not being utilized
- Or, if geist UI components are intended to be used, update the components to properly import and use them
🔗 Analysis chain
LGTM! Verify geist package setup
The addition of geist to transpilePackages is correct and follows Next.js best practices for handling external UI libraries.
Let's verify the geist package setup:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify geist package installation and configuration
# Check if geist is listed in package.json with the correct version
echo "Checking package.json for geist dependency..."
cat package.json | jq '.dependencies.geist, .devDependencies.geist'
# Look for geist imports in the codebase
echo "Checking geist usage in the codebase..."
rg "from ['|\"]geist" --type ts --type tsx
Length of output: 344
Script:
#!/bin/bash
# Let's try a different approach to verify geist setup
# Check package.json files in all directories
echo "Checking all package.json files for geist..."
fd package.json --type f --exec cat {} \; | jq -c 'select(.dependencies.geist != null or .devDependencies.geist != null)'
# Look for geist imports with broader pattern and file types
echo "Checking geist usage in the codebase..."
rg "geist" --type js --type jsx --type ts
# Check if there are any .tsx files
echo "Checking for .tsx files..."
fd -e tsx
# Check the content of next.config.js to understand the context better
echo "Content of next.config.js:"
cat apps/web/next.config.js
Length of output: 50049
Script:
#!/bin/bash
# Let's verify if geist is properly installed and used in the project
# Check all package.json files in the workspace for geist dependency
echo "Checking workspace package.json files for geist..."
fd package.json --type f --exec grep -l "geist"
# Look for geist imports with a more comprehensive pattern
echo "Checking for geist usage in the codebase..."
rg "from ['\"]geist" --type=js,jsx,ts,tsx || true
rg "import.*geist" --type=js,jsx,ts,tsx || true
# Check if there are any UI components from geist being used
echo "Checking for geist UI components usage..."
rg "Geist|<(Button|Input|Card|Modal)" --type=js,jsx,ts,tsx || true
Length of output: 656
Script:
#!/bin/bash
# Let's try one final verification with corrected file type flags
# Check the content of web/package.json to see geist version
echo "Checking web/package.json content..."
cat apps/web/package.json | jq '.dependencies.geist, .devDependencies.geist'
# Look for geist imports with individual file type flags
echo "Checking for geist usage..."
rg "from ['\"]geist" -t ts || true
rg "from ['\"]geist" -t tsx || true
rg "import.*geist" -t ts || true
rg "import.*geist" -t tsx || true
# Check for UI components with individual file type flags
echo "Checking for geist UI components..."
rg "Geist|<(Button|Input|Card|Modal)" -t ts || true
rg "Geist|<(Button|Input|Card|Modal)" -t tsx || true
Length of output: 53172
| if (!fieldContext) { | ||
| throw new Error('useFormField should be used within <FormField>'); | ||
| } |
There was a problem hiding this comment.
Add error handling for itemContext in useFormField.
In the useFormField hook, the itemContext obtained from React.useContext(FormItemContext) might be undefined if useFormField is used outside of a FormItem component. This could lead to a runtime error when destructuring { id } from itemContext.
Apply this diff to ensure itemContext is not undefined:
const fieldContext = React.useContext(FormFieldContext);
const itemContext = React.useContext(FormItemContext);
const { getFieldState, formState } = useFormContext();
if (!fieldContext) {
throw new Error('useFormField should be used within <FormField>');
}
+ if (!itemContext) {
+ throw new Error('useFormField should be used within <FormItem>');
+ }
const fieldState = getFieldState(fieldContext.name, formState);
const { id } = itemContext;📝 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.
| if (!fieldContext) { | |
| throw new Error('useFormField should be used within <FormField>'); | |
| } | |
| if (!fieldContext) { | |
| throw new Error('useFormField should be used within <FormField>'); | |
| } | |
| if (!itemContext) { | |
| throw new Error('useFormField should be used within <FormItem>'); | |
| } |
[Web] Refactor/sidebar
Please include a summary of the changes and the related issue.
Type of Change
Checklist
Previous screenshots
Screen.Recording.2024-11-21.at.22.57.47.mov
Current screenshots
Screen.Recording.2024-11-21.at.22.54.31.mov
Summary by CodeRabbit
Release Notes
New Features
SidebarOptInFormcomponent for newsletter sign-ups.Improvements
Bug Fixes
Dependencies