Add banner and sign-in features with calendar adjustments#1594
Add banner and sign-in features with calendar adjustments#1594
Conversation
📝 WalkthroughWalkthroughThis PR introduces keyboard navigation shortcuts for tabs, implements interactive billing UI, refactors settings components to use internal state, and extends tab components with a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SearchUI as Search Component
participant CmdHook as useCmdKeyPressed
participant ShortcutBadge as Shared/Tab Badge
User->>CmdHook: Press Cmd key
CmdHook->>CmdHook: Set isCmdPressed = true
SearchUI->>CmdHook: Read isCmdPressed state
SearchUI->>SearchUI: Compute showShortcut = isCmdPressed && !query
SearchUI->>User: Display ⌘ K shortcut hint
User->>ShortcutBadge: Cmd pressed, viewing tab
ShortcutBadge->>ShortcutBadge: Compute showShortcut = isCmdPressed && tabIndex !== undefined
ShortcutBadge->>User: Display ⌘ {tabIndex} badge
User->>CmdHook: Release Cmd key
CmdHook->>CmdHook: Set isCmdPressed = false
SearchUI->>User: Hide ⌘ K shortcut
ShortcutBadge->>User: Hide ⌘ {tabIndex} badge
sequenceDiagram
participant User
participant ProfileUI as Profile Sidebar
participant AuthState as Authentication State
participant WindowAPI as Windows Plugin
User->>ProfileUI: Click on Profile button
ProfileUI->>AuthState: Check isAuthenticated
alt Not Authenticated
AuthState-->>ProfileUI: Return false
ProfileUI->>User: Show Sign in button + TryProBanner
User->>ProfileUI: Click Sign in
ProfileUI->>AuthState: handleAuth() - set isAuthenticated = true
else Authenticated
AuthState-->>ProfileUI: Return true
ProfileUI->>User: Show Log out button
User->>ProfileUI: Click Log out
ProfileUI->>AuthState: handleAuth() - set isAuthenticated = false
end
User->>ProfileUI: Click Billing (from Pro trial badge)
ProfileUI->>WindowAPI: handleClickBilling()
WindowAPI->>WindowAPI: Open billing window
WindowAPI->>ProfileUI: Navigate to billing tab
sequenceDiagram
participant User
participant TemplatesUI as Settings Templates
participant LocalState as currentTemplate State
participant TemplateEditor as TemplateEditor Component
participant useTemplates as useTemplates Hook
TemplatesUI->>useTemplates: Fetch user templates
useTemplates-->>TemplatesUI: Return templates array
TemplatesUI->>TemplatesUI: Filter by searchQuery
TemplatesUI->>User: Display filtered template cards
User->>TemplatesUI: Click on template card
TemplatesUI->>LocalState: Set currentTemplate = selected
LocalState-->>TemplatesUI: Update state
TemplatesUI->>TemplateEditor: Render with currentTemplate
TemplateEditor->>User: Display editor for template
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Rationale: The PR introduces multiple cohesive but distinct features: (1) repetitive Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/components/main/body/calendars.tsx (1)
252-274: Consider extracting magic numbers as named constants.The implementation correctly measures available height and calculates visible items. However, the hardcoded values
EVENT_HEIGHT = 20andSPACING = 4could become maintenance issues if the corresponding Tailwind classes (h-5at line 391,space-y-1at line 328) are changed.Consider extracting these at the module level:
const CALENDAR_ITEM_HEIGHT = 20; // matches h-5 class const CALENDAR_ITEM_SPACING = 4; // matches space-y-1 classThen reference them in both the CSS classes (via comments) and the calculation:
const EVENT_HEIGHT = CALENDAR_ITEM_HEIGHT; const SPACING = CALENDAR_ITEM_SPACING;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/components/main/body/calendars.tsx(6 hunks)apps/desktop/src/components/main/sidebar/profile/index.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/desktop/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (apps/desktop/.cursor/rules/style.mdc)
apps/desktop/**/*.{tsx,jsx}: When there are many Tailwind classNames with conditional logic, use the utilitycnimported asimport { cn } from "@hypr/utils"
When usingcnfor Tailwind classNames, always pass an array
Group Tailwind classNames by logical sections when usingcn(split array items by grouping)
Files:
apps/desktop/src/components/main/sidebar/profile/index.tsxapps/desktop/src/components/main/body/calendars.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci (macos, macos-14)
🔇 Additional comments (8)
apps/desktop/src/components/main/body/calendars.tsx (4)
19-19: LGTM: Necessary imports for dynamic height measurement.The addition of
useEffectanduseRefsupports the new dynamic height calculation feature implemented below.
141-141: LGTM: UI refinement.The size adjustment to
"sm"improves the visual balance of the Today button within the button group.
224-226: LGTM: Proper ref and state setup.The refs and state are correctly initialized for measuring available height and calculating the maximum number of visible items dynamically.
277-279: LGTM: Dynamic visible count calculation and ref placement.The visible count calculation correctly uses the dynamic
maxVisibleItemsand reserves space for the "+x more" indicator when needed. The refs are applied to the appropriate DOM elements for accurate height measurements.Also applies to: 299-299, 328-328
apps/desktop/src/components/main/sidebar/profile/index.tsx (4)
2-2: LGTM: Required imports for authentication UI.The Button component and authentication-related icons are properly imported to support the sign-in/logout functionality.
Also applies to: 6-6
26-28: LGTM: Clear mock authentication state.The authentication state is clearly documented as mock implementation, with sensible initial values.
38-50: LGTM: Correct authentication handler logic.The function properly toggles authentication state and resets banner dismissal on logout, ensuring the banner reappears for new sign-in flows. The console logs are acceptable for mock implementation.
95-95: LGTM: Complete dependency array.The inclusion of both
isBannerDismissedandisAuthenticatedis correct, as both affect the main view height and require recalculation when changed.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/desktop/src/components/main/sidebar/profile/index.tsx (1)
202-228: Fix: Exit animation broken by conditional rendering.The banner's exit animation won't play because the parent unmounts
TryProBannerimmediately whenisBannerDismissedbecomestrue. Line 216 also hardcodesisDismissed={false}instead of passing the actual state.Apply this diff to fix the animation:
{menuItems.map((item) => <MenuItem key={item.label} {...item} />)} {isAuthenticated ? ( <div className="px-1 py-2"> <Button onClick={handleAuth} variant="outline" className="w-full" > <LogOut className="w-4 h-4 mr-2" /> Log out </Button> </div> ) - : !isBannerDismissed - ? <TryProBanner isDismissed={false} onDismiss={handleDismissBanner} /> - : ( + : ( + <> + <TryProBanner isDismissed={isBannerDismissed} onDismiss={handleDismissBanner} /> + {isBannerDismissed && ( - <div className="px-1 py-2"> - <Button - onClick={handleAuth} - variant="default" - className="w-full" - > - <LogIn className="w-4 h-4 mr-2" /> - Sign in - </Button> - </div> + <div className="px-1 py-2"> + <Button + onClick={handleAuth} + variant="default" + className="w-full" + > + <LogIn className="w-4 h-4 mr-2" /> + Sign in + </Button> + </div> + )} + </> )}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/components/main/sidebar/profile/index.tsx(9 hunks)apps/desktop/src/components/settings/integrations.tsx(3 hunks)apps/desktop/src/components/settings/template/index.tsx(2 hunks)apps/desktop/src/routes/app/settings/_layout.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/desktop/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (apps/desktop/.cursor/rules/style.mdc)
apps/desktop/**/*.{tsx,jsx}: When there are many Tailwind classNames with conditional logic, use the utilitycnimported asimport { cn } from "@hypr/utils"
When usingcnfor Tailwind classNames, always pass an array
Group Tailwind classNames by logical sections when usingcn(split array items by grouping)
Files:
apps/desktop/src/components/settings/template/index.tsxapps/desktop/src/routes/app/settings/_layout.tsxapps/desktop/src/components/main/sidebar/profile/index.tsxapps/desktop/src/components/settings/integrations.tsx
🧬 Code graph analysis (3)
apps/desktop/src/components/settings/template/index.tsx (2)
packages/db/src/schema.ts (1)
templates(101-106)apps/desktop/src/components/settings/template/editor.tsx (1)
TemplateEditor(20-209)
apps/desktop/src/components/main/sidebar/profile/index.tsx (1)
apps/desktop/src/components/main/sidebar/profile/banner.tsx (1)
TryProBanner(7-70)
apps/desktop/src/components/settings/integrations.tsx (1)
apps/desktop/src/components/main/body/search.tsx (1)
Search(7-62)
🪛 Biome (2.1.2)
apps/desktop/src/components/settings/template/index.tsx
[error] 29-29: filtered is assigned to itself.
This is where is assigned.
Self assignments have no effect and can be removed.
(lint/correctness/noSelfAssign)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci (macos, macos-14)
🔇 Additional comments (9)
apps/desktop/src/routes/app/settings/_layout.tsx (1)
64-64: LGTM!The
scrollbar-noneclass appropriately hides the scrollbar for a cleaner settings layout.apps/desktop/src/components/settings/integrations.tsx (2)
86-114: LGTM!The filtering logic is well-structured with clear separation between status and search filters. The memoization includes all necessary dependencies.
131-156: LGTM!The ButtonGroup implementation provides a clean UI for status filtering and follows consistent patterns.
apps/desktop/src/components/settings/template/index.tsx (2)
32-43: LGTM!The search filtering logic is well-implemented with proper memoization and dependency tracking.
54-106: LGTM!The filter UI and template rendering logic follow consistent patterns and provide a clean user experience with appropriate empty states.
apps/desktop/src/components/main/sidebar/profile/index.tsx (4)
26-28: Reminder: Mock authentication state for testing.The mock
isAuthenticatedstate should be replaced with actual authentication logic before production deployment.Are you planning to integrate real authentication in a follow-up PR?
38-50: LGTM!The authentication handler correctly manages state transitions and resets the banner on logout.
141-151: LGTM!The profile click handler opens the contacts view appropriately. The TODO comment correctly notes that this should eventually show the user's own profile.
261-263: LGTM!The
onClickBillingprop integration is well-implemented with proper event propagation handling on the Pro trial badge.Also applies to: 297-310
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/components/main/body/sessions/floating/listen.tsx (1)
144-151: CompleteuseRemoteMeetingto detect meeting platform from session data.The function is indeed a stub—it ignores the
_sessionIdparameter (indicated by the underscore prefix) and always returnsgoogle-meet. However, the caller inBeforeMeeingButtonexpects detection of four meeting types (zoom, google-meet, webex, teams). The function must be implemented to determine the actual meeting platform based on the session ID and return the corresponding type.
🧹 Nitpick comments (3)
apps/desktop/src/components/main/body/sessions/floating/listen.tsx (1)
126-126: Apply the same size prop pattern for consistency.This Icon still uses className-based sizing (
w-5 h-5) while the other Icon components in this file now use thesizeprop. For consistency, usesize={20}and keep className only for the margin.Apply this diff:
- icon={hovered ? <Icon icon="lucide:stop-circle" className="w-5 h-5 mt-1.5" /> : undefined} + icon={hovered ? <Icon icon="lucide:stop-circle" size={20} className="mt-1.5" /> : undefined}apps/desktop/src/hooks/useCmdKeyPressed.tsx (1)
17-21: Optimize handleKeyUp to prevent unnecessary state updates.The condition
!e.metaKey || e.key === "Meta"will triggersetIsCmdPressed(false)on every key release when Cmd is not held (e.g., typing normal text). This causes unnecessary re-renders across all components using this hook.Apply this diff to only reset state when the Meta key itself is released:
const handleKeyUp = (e: KeyboardEvent) => { - if (!e.metaKey || e.key === "Meta") { + if (e.key === "Meta") { setIsCmdPressed(false); } };The
handleBluralready covers the case where the window loses focus while Cmd is held.apps/desktop/src/components/main/body/calendars.tsx (1)
226-228: Excellent dynamic height calculation implementation!The per-day height measurement and dynamic
maxVisibleItemscalculation is a solid improvement over static limits. The implementation correctly:
- Measures actual DOM dimensions using refs
- Calculates available space and fits items accordingly
- Reserves space for the "+x more" indicator when needed
- Cleans up the resize listener properly
- Uses
Math.max(1, ...)to ensure at least one item is always visibleOptional: Extract magic numbers as named constants.
The values
EVENT_HEIGHT = 20andSPACING = 4are correct (matchingh-5andspace-y-1Tailwind classes), but they're tightly coupled to the CSS. Consider extracting these as module-level constants for better maintainability:+// Calendar day cell item dimensions +const EVENT_ITEM_HEIGHT = 20; // matches h-5 (1.25rem) +const EVENT_ITEM_SPACING = 4; // matches space-y-1 (0.25rem) + function TabContentCalendarDay({ // ... }) { // ... useEffect(() => { const measureHeight = () => { if (cellRef.current && contentRef.current) { const cellHeight = cellRef.current.clientHeight; const contentTop = contentRef.current.offsetTop; const availableHeight = cellHeight - contentTop; - const EVENT_HEIGHT = 20; // height of each event item (h-5) - const SPACING = 4; // space-y-1 // Calculate how many items can fit - const itemsWithSpacing = Math.floor((availableHeight + SPACING) / (EVENT_HEIGHT + SPACING)); + const itemsWithSpacing = Math.floor((availableHeight + EVENT_ITEM_SPACING) / (EVENT_ITEM_HEIGHT + EVENT_ITEM_SPACING)); // Reserve space for "+x more" if needed setMaxVisibleItems(Math.max(1, itemsWithSpacing)); } };This makes it easier to update if the CSS classes change and improves code clarity.
Also applies to: 254-276, 279-281, 301-301, 330-330
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/desktop/src/components/main/body/calendars.tsx(8 hunks)apps/desktop/src/components/main/body/contacts/index.tsx(2 hunks)apps/desktop/src/components/main/body/events.tsx(2 hunks)apps/desktop/src/components/main/body/folders.tsx(4 hunks)apps/desktop/src/components/main/body/humans.tsx(2 hunks)apps/desktop/src/components/main/body/index.tsx(8 hunks)apps/desktop/src/components/main/body/search.tsx(3 hunks)apps/desktop/src/components/main/body/sessions/floating/listen.tsx(4 hunks)apps/desktop/src/components/main/body/sessions/index.tsx(2 hunks)apps/desktop/src/components/main/body/shared.tsx(4 hunks)apps/desktop/src/hooks/useCmdKeyPressed.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/desktop/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (apps/desktop/.cursor/rules/style.mdc)
apps/desktop/**/*.{tsx,jsx}: When there are many Tailwind classNames with conditional logic, use the utilitycnimported asimport { cn } from "@hypr/utils"
When usingcnfor Tailwind classNames, always pass an array
Group Tailwind classNames by logical sections when usingcn(split array items by grouping)
Files:
apps/desktop/src/components/main/body/events.tsxapps/desktop/src/components/main/body/contacts/index.tsxapps/desktop/src/components/main/body/humans.tsxapps/desktop/src/components/main/body/search.tsxapps/desktop/src/components/main/body/sessions/index.tsxapps/desktop/src/hooks/useCmdKeyPressed.tsxapps/desktop/src/components/main/body/sessions/floating/listen.tsxapps/desktop/src/components/main/body/index.tsxapps/desktop/src/components/main/body/calendars.tsxapps/desktop/src/components/main/body/folders.tsxapps/desktop/src/components/main/body/shared.tsx
🧬 Code graph analysis (3)
apps/desktop/src/components/main/body/search.tsx (2)
apps/desktop/src/contexts/search/ui.tsx (1)
useSearch(219-225)apps/desktop/src/hooks/useCmdKeyPressed.tsx (1)
useCmdKeyPressed(7-40)
apps/desktop/src/components/main/body/index.tsx (2)
apps/desktop/src/store/zustand/tabs/schema.ts (2)
uniqueIdfromTab(86-103)Tab(54-54)apps/desktop/src/components/main/body/shared.tsx (1)
TabItem(26-26)
apps/desktop/src/components/main/body/shared.tsx (2)
apps/desktop/src/hooks/useCmdKeyPressed.tsx (1)
useCmdKeyPressed(7-40)packages/ui/src/components/ui/kbd.tsx (2)
KbdGroup(28-28)Kbd(28-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci (macos, macos-14)
🔇 Additional comments (13)
apps/desktop/src/components/main/body/sessions/floating/listen.tsx (1)
54-54: LGTM! Icon sizing is now consistent.Using the
sizeprop instead of Tailwind className-based sizing (w-5 h-5) improves consistency and leverages the Icon component's dedicated sizing API.Also applies to: 65-65, 76-76, 87-87
apps/desktop/src/components/main/body/search.tsx (1)
12-70: LGTM! Clean keyboard shortcut integration.The implementation correctly:
- Tracks Cmd key state via the hook
- Dynamically adjusts input padding to prevent overlap with the shortcut badge and clear button
- Shows the ⌘ K hint only when appropriate (Cmd pressed and no query)
- Follows coding guidelines by using
cnwith logically grouped arraysapps/desktop/src/components/main/body/shared.tsx (1)
12-100: Keyboard shortcut feature implementation looks solid.The tabIndex prop threading and conditional shortcut badge rendering are well-implemented:
- Clean prop typing with optional
tabIndex- Efficient conditional rendering based on Cmd key state
- Good use of
pointer-events-noneto prevent interaction interference- Appropriate positioning for the shortcut badge
apps/desktop/src/components/main/body/contacts/index.tsx (1)
18-30: LGTM! Consistent tabIndex prop threading.The
tabIndexprop is correctly added to the component signature and forwarded toTabItemBase, consistent with the pattern used across all TabItem variants in this PR.apps/desktop/src/components/main/body/sessions/index.tsx (1)
19-33: LGTM! Consistent tabIndex prop threading.The
tabIndexprop is correctly added to the component signature and forwarded toTabItemBase, consistent with the pattern used across all TabItem variants in this PR.apps/desktop/src/components/main/body/events.tsx (1)
11-25: LGTM! Consistent tabIndex prop threading.The
tabIndexprop is correctly added to the component signature and forwarded toTabItemBase, consistent with the pattern used across all TabItem variants in this PR.apps/desktop/src/components/main/body/folders.tsx (2)
25-37: LGTM! Consistent tabIndex prop threading in TabItemFolderAll.The
tabIndexprop is correctly added to the component signature and forwarded toTabItemBase, consistent with the pattern used across all TabItem variants in this PR.
48-65: LGTM! Consistent tabIndex prop threading in TabItemFolderSpecific.The
tabIndexprop is correctly added to the component signature and forwarded toTabItemBase, consistent with the pattern used across all TabItem variants in this PR.apps/desktop/src/components/main/body/humans.tsx (1)
10-27: LGTM! Consistent tabIndex prop threading.The
tabIndexprop is correctly added to the component signature and forwarded toTabItemBase, consistent with the pattern used across all TabItem variants in this PR.apps/desktop/src/components/main/body/calendars.tsx (2)
26-48: LGTM! Keyboard navigation support added.The
tabIndexprop is properly accepted and forwarded toTabItemBase, enabling keyboard shortcuts for calendar tabs. This aligns with the broader tabIndex propagation pattern implemented across all tab components.
143-143: LGTM! Button size adjustment improves visual consistency.Using
size="sm"provides better visual balance in the header alongside the icon buttons.apps/desktop/src/components/main/body/index.tsx (2)
96-120: LGTM! Keyboard shortcut indices correctly calculated.The
shortcutIndexlogic properly maps:
- First 8 tabs → Cmd+1 through Cmd+8
- Last tab → Cmd+9 (when beyond the first 8)
- Intermediate tabs → no shortcut (undefined)
This aligns perfectly with the keyboard shortcut handler in
useTabsShortcuts(lines 352-364).
143-234: LGTM! Consistent tabIndex propagation across all tab types.The
TabItemfunction properly accepts the optionaltabIndexparameter and forwards it to all six tab variants (sessions, events, folders, humans, calendars, contacts). This ensures uniform keyboard navigation support across the entire tab system.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/components/settings/billing.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/desktop/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (apps/desktop/.cursor/rules/style.mdc)
apps/desktop/**/*.{tsx,jsx}: When there are many Tailwind classNames with conditional logic, use the utilitycnimported asimport { cn } from "@hypr/utils"
When usingcnfor Tailwind classNames, always pass an array
Group Tailwind classNames by logical sections when usingcn(split array items by grouping)
Files:
apps/desktop/src/components/settings/billing.tsx
🧬 Code graph analysis (1)
apps/desktop/src/components/settings/billing.tsx (1)
packages/utils/src/cn.ts (1)
cn(20-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci (macos, macos-14)
🔇 Additional comments (3)
apps/desktop/src/components/settings/billing.tsx (3)
59-59: Verify if the hardcoded initial plan is intentional.The current plan is hardcoded to
"free". In a production app, this should typically be fetched from user data or an API.
70-77: Verify stub implementations for plan changes and contact.Both handlers only perform local actions (console logging and opening a URL) without backend integration or error handling. Confirm whether:
- Plan changes should trigger API calls or state persistence
- URL opening failures should provide user feedback
176-228: LGTM!The conditional logic correctly handles all plan state transitions with appropriate button labels and variants.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/components/main/sidebar/profile/index.tsx (1)
269-324: Use arrays withcn()utility as per coding guidelines.The coding guidelines specify: "When using
cnfor Tailwind classNames, always pass an array" and "Group Tailwind classNames by logical sections when usingcn(split array items by grouping)". All fourcn()calls should be updated to use arrays.As per coding guidelines
Apply this diff:
<button - className={cn( - "flex w-full items-center gap-2.5", - "px-4 py-2", - "text-left", - "transition-all duration-300", - "hover:bg-neutral-100", - isExpanded && "bg-neutral-50 border-t border-neutral-100", + className={cn([ + "flex w-full items-center gap-2.5", + "px-4 py-2", + "text-left", + "transition-all duration-300", + "hover:bg-neutral-100", + isExpanded && "bg-neutral-50 border-t border-neutral-100", + ])} - )} onClick={onClick} > <div - className={cn( - "flex size-8 flex-shrink-0 items-center justify-center", - "overflow-hidden rounded-full", - "border border-white/60 border-t border-neutral-400", - "bg-gradient-to-br from-indigo-400 to-purple-500", - "shadow-sm", - "transition-transform duration-300", + className={cn([ + "flex size-8 flex-shrink-0 items-center justify-center", + "overflow-hidden rounded-full", + "border border-white/60 border-t border-neutral-400", + "bg-gradient-to-br from-indigo-400 to-purple-500", + "shadow-sm", + "transition-transform duration-300", + ])} - )} > <img src="https://api.dicebear.com/7.x/avataaars/svg?seed=JohnJeong" alt="Profile" className="h-full w-full rounded-full" /> </div> <div className="min-w-0 flex-1"> <div className="text-sm text-black truncate">John Jeong</div> </div> <div className="flex items-center gap-1.5"> <span onClick={(e) => { e.stopPropagation(); onClickBilling(); }} - className={cn( - "hidden md:inline-block", - "rounded-full", - "border border-neutral-900", - "bg-white", - "px-2.5 py-0.5", - "text-[11px] font-medium text-neutral-900", - "cursor-pointer", - "hover:bg-neutral-100", - "transition-colors duration-200", + className={cn([ + "hidden md:inline-block", + "rounded-full", + "border border-neutral-900", + "bg-white", + "px-2.5 py-0.5", + "text-[11px] font-medium text-neutral-900", + "cursor-pointer", + "hover:bg-neutral-100", + "transition-colors duration-200", + ])} - )} > Pro trial </span> <ChevronUpIcon - className={cn( - "h-4 w-4", - "transition-transform duration-300", - isExpanded ? "rotate-180 text-neutral-500" : "text-neutral-400", + className={cn([ + "h-4 w-4", + "transition-transform duration-300", + isExpanded ? "rotate-180 text-neutral-500" : "text-neutral-400", + ])} - )} /> </div> </button>
♻️ Duplicate comments (3)
apps/desktop/src/components/main/sidebar/profile/index.tsx (1)
104-110: MovecloseMenu()into the promise chain.This is the same timing issue flagged in the previous review. The menu closes before navigation completes because
closeMenu()executes synchronously. Move it into the.then()callback:const handleClickBilling = useCallback(() => { windowsCommands.windowShow({ type: "settings" }).then(() => { windowsCommands.windowEmitNavigate({ type: "settings" }, { path: "/app/settings", search: { tab: "billing" } }); + closeMenu(); }); - closeMenu(); }, [closeMenu]);apps/desktop/src/components/settings/billing.tsx (2)
137-144: Group Tailwind classes by logical sections within cn.You switched to an array (good), but background is mixed with layout. Group for clarity and consistency.
As per coding guidelines.
- className={cn( - [ - "p-6 flex flex-col gap-6 bg-white", - !removeBorder && "border border-neutral-200 rounded-lg", - plan.id === "ultimate" && "bg-neutral-50", - className, - ], - )} + className={cn([ + // Layout & spacing + "p-6 flex flex-col gap-6", + // Background + "bg-white", + plan.id === "ultimate" && "bg-neutral-50", + // Border + !removeBorder && "border border-neutral-200 rounded-lg", + // Custom overrides + className, + ])}
90-90: Pass an array to cn.cn should always receive a single array. Wrap the conditional class in an array.
As per coding guidelines.
- className={cn(index === 0 && "border-r border-neutral-200")} + className={cn([index === 0 && "border-r border-neutral-200"])}
🧹 Nitpick comments (4)
apps/desktop/src/components/settings/template/index.tsx (1)
140-140: Fix typo in variable name.The variable is named
quriesinstead ofqueries. While this is pre-existing code, it's a simple fix that improves readability.Apply this diff:
- const quries = persisted.UI.useCreateQueries( + const queries = persisted.UI.useCreateQueries(And update line 152:
- const templates = persisted.UI.useResultTable(USER_TEMPLATE_QUERY, quries); + const templates = persisted.UI.useResultTable(USER_TEMPLATE_QUERY, queries);apps/desktop/src/components/main/sidebar/profile/index.tsx (1)
141-151: Address TODO: Show user's own profile.The TODO indicates that the current implementation opens the contacts view but doesn't specifically show the user's own profile. Consider implementing logic to select/highlight the current user's profile when this action is triggered.
Would you like me to help generate the implementation that pre-selects the user's profile in the contacts view?
apps/desktop/src/components/settings/billing.tsx (2)
61-68: Avoid useMemo for constants; hoist derived plans.Derived values from a file‑level constant don’t need useMemo([]). Hoist once and reference directly; also removes the non‑null assertion in render paths.
Apply inside component:
- const standardPlans = useMemo( - () => PLANS.filter((plan) => plan.id !== "ultimate"), - [], - ); - const ultimatePlan = useMemo( - () => PLANS.find((plan) => plan.id === "ultimate")!, - [], - ); + const standardPlans = STANDARD_PLANS; + const ultimatePlan = ULTIMATE_PLAN;Add near PLANS (file scope):
const STANDARD_PLANS: BillingPlan[] = PLANS.filter((p) => p.id !== "ultimate"); const ULTIMATE_PLAN = PLANS.find((p) => p.id === "ultimate")!;
152-158: Mark decorative icons aria-hidden.The checkmark is decorative; hide it from screen readers.
- <Check size={16} className="mt-0.5 text-emerald-500 shrink-0" /> + <Check size={16} aria-hidden="true" focusable="false" className="mt-0.5 text-emerald-500 shrink-0" />
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/components/main/body/shared.tsx(5 hunks)apps/desktop/src/components/main/sidebar/profile/index.tsx(10 hunks)apps/desktop/src/components/settings/billing.tsx(1 hunks)apps/desktop/src/components/settings/template/index.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/components/main/body/shared.tsx
🧰 Additional context used
📓 Path-based instructions (1)
apps/desktop/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (apps/desktop/.cursor/rules/style.mdc)
apps/desktop/**/*.{tsx,jsx}: When there are many Tailwind classNames with conditional logic, use the utilitycnimported asimport { cn } from "@hypr/utils"
When usingcnfor Tailwind classNames, always pass an array
Group Tailwind classNames by logical sections when usingcn(split array items by grouping)
Files:
apps/desktop/src/components/main/sidebar/profile/index.tsxapps/desktop/src/components/settings/template/index.tsxapps/desktop/src/components/settings/billing.tsx
🧠 Learnings (3)
📚 Learning: 2025-10-21T03:03:20.070Z
Learnt from: CR
PR: fastrepl/hyprnote#0
File: apps/desktop/.cursor/rules/style.mdc:0-0
Timestamp: 2025-10-21T03:03:20.070Z
Learning: Applies to apps/desktop/**/*.{tsx,jsx} : When using `cn` for Tailwind classNames, always pass an array
Applied to files:
apps/desktop/src/components/settings/billing.tsx
📚 Learning: 2025-10-21T03:03:20.070Z
Learnt from: CR
PR: fastrepl/hyprnote#0
File: apps/desktop/.cursor/rules/style.mdc:0-0
Timestamp: 2025-10-21T03:03:20.070Z
Learning: Applies to apps/desktop/**/*.{tsx,jsx} : When there are many Tailwind classNames with conditional logic, use the utility `cn` imported as `import { cn } from "hypr/utils"`
Applied to files:
apps/desktop/src/components/settings/billing.tsx
📚 Learning: 2025-10-21T03:03:20.070Z
Learnt from: CR
PR: fastrepl/hyprnote#0
File: apps/desktop/.cursor/rules/style.mdc:0-0
Timestamp: 2025-10-21T03:03:20.070Z
Learning: Applies to apps/desktop/**/*.{tsx,jsx} : Group Tailwind classNames by logical sections when using `cn` (split array items by grouping)
Applied to files:
apps/desktop/src/components/settings/billing.tsx
🧬 Code graph analysis (3)
apps/desktop/src/components/main/sidebar/profile/index.tsx (2)
apps/desktop/src/components/main/sidebar/profile/banner.tsx (1)
TryProBanner(7-70)packages/utils/src/cn.ts (1)
cn(20-22)
apps/desktop/src/components/settings/template/index.tsx (2)
packages/db/src/schema.ts (1)
templates(101-106)apps/desktop/src/components/settings/template/editor.tsx (1)
TemplateEditor(20-209)
apps/desktop/src/components/settings/billing.tsx (2)
packages/utils/src/cn.ts (1)
cn(20-22)packages/ui/src/components/ui/button.tsx (1)
Button(53-53)
🔇 Additional comments (9)
apps/desktop/src/components/settings/template/index.tsx (2)
26-27: Past issue resolved.The no-op self-assignment flagged in the previous review has been correctly removed. Only the TODO comment remains, which is appropriate.
120-124: Excellent adherence to coding guidelines.The
cnusage correctly follows the guidelines by using array format and grouping Tailwind classes by logical sections (layout, interaction, and visual styling). The change fromitems-centertoitems-startalso better accommodates multi-line descriptions.As per coding guidelines
apps/desktop/src/components/main/sidebar/profile/index.tsx (6)
2-6: LGTM!The import additions are appropriate and all imported items are used in the component.
26-28: Replace mock authentication with real auth integration.The mock authentication state is suitable for testing but should be replaced with actual authentication logic (e.g., from an auth provider or service) before production deployment.
38-50: Replace mock auth handlers with real authentication.The console.log statements should be replaced with actual authentication calls (sign-in/sign-out). The logic for resetting the banner on logout is appropriate.
95-95: LGTM!Correctly includes
isBannerDismissedandisAuthenticateddependencies, as changes to these states affect the rendered content height.
202-231: LGTM! Exit animation fix has been applied correctly.The banner rendering structure now matches the suggested fix from the previous review. The
TryProBanneris always rendered when the user is not authenticated and receives theisDismissedprop, allowing its internalAnimatePresenceexit animation to play correctly before the sign-in button appears.
254-258: LGTM!The
ProfileButtonusage correctly passes theonClickBillinghandler, enabling the billing navigation feature.apps/desktop/src/components/settings/billing.tsx (1)
178-230: PlanActions logic reads clean and covers all states.Early return for current plan and clear upgrade/downgrade labels look good.
ComputelessComputer
force-pushed
the
jj-branch-26
branch
from
9d30560 to
98581e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/desktop/src/components/settings/template/index.tsx (1)
140-140: Fix typo in variable name.The variable name
quriesshould bequeries.Apply this diff:
- const quries = persisted.UI.useCreateQueries( + const queries = persisted.UI.useCreateQueries( store, (store) => createQueries(store).setQueryDefinition(USER_TEMPLATE_QUERY, "templates", ({ select, where }) => { select("title"); select("description"); select("sections"); where("user_id", user_id ?? ""); }), [user_id], ); - const templates = persisted.UI.useResultTable(USER_TEMPLATE_QUERY, quries); + const templates = persisted.UI.useResultTable(USER_TEMPLATE_QUERY, queries); return templates as unknown as Record<string, persisted.Template>;apps/desktop/src/components/main/sidebar/profile/index.tsx (3)
269-276: Pass an array tocnand group classNames per coding guidelines.The coding guidelines require: "When using
cnfor Tailwind classNames, always pass an array" and "Group Tailwind classNames by logical sections".As per coding guidelines.
Apply this diff:
- className={cn( - "flex w-full items-center gap-2.5", - "px-4 py-2", - "text-left", - "transition-all duration-300", - "hover:bg-neutral-100", - isExpanded && "bg-neutral-50 border-t border-neutral-100", - )} + className={cn([ + // Layout + "flex w-full items-center gap-2.5", + "px-4 py-2", + "text-left", + // Interactions + "transition-all duration-300", + "hover:bg-neutral-100", + isExpanded && "bg-neutral-50 border-t border-neutral-100", + ])}
280-287: Pass an array tocnand group classNames per coding guidelines.As per coding guidelines.
Apply this diff:
- className={cn( - "flex size-8 flex-shrink-0 items-center justify-center", - "overflow-hidden rounded-full", - "border border-white/60 border-t border-neutral-400", - "bg-gradient-to-br from-indigo-400 to-purple-500", - "shadow-sm", - "transition-transform duration-300", - )} + className={cn([ + // Layout + "flex size-8 flex-shrink-0 items-center justify-center", + "overflow-hidden rounded-full", + // Styling + "border border-white/60 border-t border-neutral-400", + "bg-gradient-to-br from-indigo-400 to-purple-500", + "shadow-sm", + "transition-transform duration-300", + ])}
319-323: Pass an array tocnper coding guidelines.As per coding guidelines.
Apply this diff:
- className={cn( - "h-4 w-4", - "transition-transform duration-300", - isExpanded ? "rotate-180 text-neutral-500" : "text-neutral-400", - )} + className={cn([ + "h-4 w-4", + "transition-transform duration-300", + isExpanded ? "rotate-180 text-neutral-500" : "text-neutral-400", + ])}
♻️ Duplicate comments (4)
apps/desktop/src/components/settings/billing.tsx (2)
90-90: Pass an array tocnper coding guidelines.The coding guidelines specify: "When using
cnfor Tailwind classNames, always pass an array".As per coding guidelines.
137-144: Group classNames by logical sections per coding guidelines.While the array wrapper is now present, the coding guidelines also specify: "Group Tailwind classNames by logical sections when using
cn(split array items by grouping)".As per coding guidelines.
apps/desktop/src/components/main/sidebar/profile/index.tsx (2)
104-110: MovecloseMenu()into the promise chain.The TODO comment is valid—
closeMenu()executes synchronously while the navigation is still pending, which can cause timing issues.
202-231: The TryProBanner animation handling appears correct.The current implementation keeps
TryProBannermounted when not authenticated and passes theisDismissedprop, allowing the internalAnimatePresenceto handle exit animations properly. This addresses the concern raised in previous reviews.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/components/main/body/shared.tsx(5 hunks)apps/desktop/src/components/main/sidebar/profile/index.tsx(10 hunks)apps/desktop/src/components/settings/billing.tsx(1 hunks)apps/desktop/src/components/settings/template/index.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/components/main/body/shared.tsx
🧰 Additional context used
📓 Path-based instructions (1)
apps/desktop/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (apps/desktop/.cursor/rules/style.mdc)
apps/desktop/**/*.{tsx,jsx}: When there are many Tailwind classNames with conditional logic, use the utilitycnimported asimport { cn } from "@hypr/utils"
When usingcnfor Tailwind classNames, always pass an array
Group Tailwind classNames by logical sections when usingcn(split array items by grouping)
Files:
apps/desktop/src/components/settings/template/index.tsxapps/desktop/src/components/settings/billing.tsxapps/desktop/src/components/main/sidebar/profile/index.tsx
🧠 Learnings (3)
📚 Learning: 2025-10-21T03:03:20.070Z
Learnt from: CR
PR: fastrepl/hyprnote#0
File: apps/desktop/.cursor/rules/style.mdc:0-0
Timestamp: 2025-10-21T03:03:20.070Z
Learning: Applies to apps/desktop/**/*.{tsx,jsx} : When using `cn` for Tailwind classNames, always pass an array
Applied to files:
apps/desktop/src/components/settings/billing.tsx
📚 Learning: 2025-10-21T03:03:20.070Z
Learnt from: CR
PR: fastrepl/hyprnote#0
File: apps/desktop/.cursor/rules/style.mdc:0-0
Timestamp: 2025-10-21T03:03:20.070Z
Learning: Applies to apps/desktop/**/*.{tsx,jsx} : When there are many Tailwind classNames with conditional logic, use the utility `cn` imported as `import { cn } from "hypr/utils"`
Applied to files:
apps/desktop/src/components/settings/billing.tsx
📚 Learning: 2025-10-21T03:03:20.070Z
Learnt from: CR
PR: fastrepl/hyprnote#0
File: apps/desktop/.cursor/rules/style.mdc:0-0
Timestamp: 2025-10-21T03:03:20.070Z
Learning: Applies to apps/desktop/**/*.{tsx,jsx} : Group Tailwind classNames by logical sections when using `cn` (split array items by grouping)
Applied to files:
apps/desktop/src/components/settings/billing.tsx
🧬 Code graph analysis (3)
apps/desktop/src/components/settings/template/index.tsx (2)
packages/db/src/schema.ts (1)
templates(101-106)apps/desktop/src/components/settings/template/editor.tsx (1)
TemplateEditor(20-209)
apps/desktop/src/components/settings/billing.tsx (1)
packages/utils/src/cn.ts (1)
cn(20-22)
apps/desktop/src/components/main/sidebar/profile/index.tsx (3)
packages/ui/src/components/ui/button.tsx (1)
Button(53-53)apps/desktop/src/components/main/sidebar/profile/banner.tsx (1)
TryProBanner(7-70)packages/utils/src/cn.ts (1)
cn(20-22)
🔇 Additional comments (2)
apps/desktop/src/components/settings/template/index.tsx (2)
23-39: LGTM! Past review issues have been addressed.The filtering logic is correct:
- The no-op self-assignment has been removed
filterStatushas been correctly removed from the dependency array- Search filtering properly checks both title and description fields with null coalescing
108-132: LGTM! Proper use ofcnutility per coding guidelines.The
cnutility is used correctly with:
- Array syntax as required
- Logical grouping of Tailwind classes (layout, interaction, styling)
As per coding guidelines.
ComputelessComputer
force-pushed
the
jj-branch-26
branch
from
98581e1 to
feca872
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/desktop/src/components/settings/template/index.tsx (1)
49-67: Remove non-functional favorite filter UI or implement the feature.The ButtonGroup with All/Favorite buttons is still present but has no effect on the filtered results. Clicking these buttons updates
filterStatusstate (line 19), but this state is never used in the filtering logic (lines 22-38). This creates a confusing user experience where the UI appears interactive but produces no results.Either remove the entire ButtonGroup and the
FilterStatustype definition (line 14) andfilterStatusstate (line 19), or implement the favorite filtering logic in theuseMemoblock.Apply this diff to remove the non-functional UI:
-type FilterStatus = "all" | "favorite"; - export function SettingsTemplates() { const [currentTemplate, setCurrentTemplate] = useState<string | null>(null); const [searchQuery, setSearchQuery] = useState(""); - const [filterStatus, setFilterStatus] = useState<FilterStatus>("all"); const templates = useTemplates();And remove the ButtonGroup from the JSX:
<div className="flex items-center justify-between mb-4"> <h2 className="font-semibold cursor-default">Templates</h2> - <ButtonGroup> - <Button - variant={filterStatus === "all" ? "default" : "outline"} - size="sm" - onClick={() => setFilterStatus("all")} - className="shadow-none" - > - All - </Button> - <Button - variant={filterStatus === "favorite" ? "default" : "outline"} - size="sm" - onClick={() => setFilterStatus("favorite")} - className="shadow-none" - > - Favorite - </Button> - </ButtonGroup> </div>Also remove the unused ButtonGroup import:
import { Button } from "@hypr/ui/components/ui/button"; -import { ButtonGroup } from "@hypr/ui/components/ui/button-group"; import { Input } from "@hypr/ui/components/ui/input";apps/desktop/src/components/settings/billing.tsx (1)
90-90: Pass an array tocnper coding guidelines.This issue was flagged in a previous review but has not been addressed. The coding guidelines specify: "When using
cnfor Tailwind classNames, always pass an array".As per coding guidelines.
Apply this diff:
- className={cn(index === 0 && "border-r border-neutral-200")} + className={cn([index === 0 && "border-r border-neutral-200"])}
🧹 Nitpick comments (2)
apps/desktop/src/components/settings/template/index.tsx (1)
69-78: Consider adding accessibility attributes to the search input.The search input would benefit from an
aria-labelattribute to improve screen reader accessibility, especially since the placeholder text may not always be announced.Apply this diff:
<Input type="text" placeholder="Search templates..." + aria-label="Search templates" value={searchQuery} onChange={(e) => setSearchQuery(e.target.value)} className="pl-9 shadow-none" />apps/desktop/src/components/settings/billing.tsx (1)
137-144: Complete the logical grouping of classNames per coding guidelines.The array is now being passed to
cn(✓), but the coding guidelines also specify: "Group Tailwind classNames by logical sections when usingcn(split array items by grouping)". The previous review suggested splitting classes by layout/spacing, background, border, and custom overrides.As per coding guidelines.
Consider applying this diff for clearer logical grouping:
className={cn( [ - "p-6 flex flex-col gap-6 bg-white", + // Layout and spacing + "p-6 flex flex-col gap-6", + // Background + "bg-white", + plan.id === "ultimate" && "bg-neutral-50", + // Border !removeBorder && "border border-neutral-200 rounded-lg", - plan.id === "ultimate" && "bg-neutral-50", + // Custom overrides className, ], )}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/components/main/body/shared.tsx(5 hunks)apps/desktop/src/components/main/sidebar/profile/index.tsx(10 hunks)apps/desktop/src/components/settings/billing.tsx(1 hunks)apps/desktop/src/components/settings/template/index.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/components/main/body/shared.tsx
- apps/desktop/src/components/main/sidebar/profile/index.tsx
🧰 Additional context used
📓 Path-based instructions (1)
apps/desktop/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (apps/desktop/.cursor/rules/style.mdc)
apps/desktop/**/*.{tsx,jsx}: When there are many Tailwind classNames with conditional logic, use the utilitycnimported asimport { cn } from "@hypr/utils"
When usingcnfor Tailwind classNames, always pass an array
Group Tailwind classNames by logical sections when usingcn(split array items by grouping)
Files:
apps/desktop/src/components/settings/template/index.tsxapps/desktop/src/components/settings/billing.tsx
🧠 Learnings (3)
📚 Learning: 2025-10-21T03:03:20.070Z
Learnt from: CR
PR: fastrepl/hyprnote#0
File: apps/desktop/.cursor/rules/style.mdc:0-0
Timestamp: 2025-10-21T03:03:20.070Z
Learning: Applies to apps/desktop/**/*.{tsx,jsx} : When using `cn` for Tailwind classNames, always pass an array
Applied to files:
apps/desktop/src/components/settings/billing.tsx
📚 Learning: 2025-10-21T03:03:20.070Z
Learnt from: CR
PR: fastrepl/hyprnote#0
File: apps/desktop/.cursor/rules/style.mdc:0-0
Timestamp: 2025-10-21T03:03:20.070Z
Learning: Applies to apps/desktop/**/*.{tsx,jsx} : When there are many Tailwind classNames with conditional logic, use the utility `cn` imported as `import { cn } from "hypr/utils"`
Applied to files:
apps/desktop/src/components/settings/billing.tsx
📚 Learning: 2025-10-21T03:03:20.070Z
Learnt from: CR
PR: fastrepl/hyprnote#0
File: apps/desktop/.cursor/rules/style.mdc:0-0
Timestamp: 2025-10-21T03:03:20.070Z
Learning: Applies to apps/desktop/**/*.{tsx,jsx} : Group Tailwind classNames by logical sections when using `cn` (split array items by grouping)
Applied to files:
apps/desktop/src/components/settings/billing.tsx
🧬 Code graph analysis (2)
apps/desktop/src/components/settings/template/index.tsx (2)
packages/db/src/schema.ts (1)
templates(101-106)apps/desktop/src/components/settings/template/editor.tsx (1)
TemplateEditor(20-209)
apps/desktop/src/components/settings/billing.tsx (1)
packages/utils/src/cn.ts (1)
cn(20-22)
🔇 Additional comments (3)
apps/desktop/src/components/settings/template/index.tsx (2)
22-38: Filtering logic correctly implemented.The search filter works correctly, and the previous issues have been resolved:
- The no-op self-assignment has been removed
filterStatusis correctly excluded from the dependency array since favorite filtering isn't implemented yet
119-123: LGTM:cnusage follows coding guidelines.The use of
cnwith an array and logical grouping of Tailwind classes follows the project's coding guidelines correctly.Based on coding guidelines.
apps/desktop/src/components/settings/billing.tsx (1)
178-230: LGTM! Clean conditional logic for plan actions.The button logic correctly handles all plan transitions:
- Shows disabled "Current Plan" when on the selected plan
- Shows appropriate upgrade/downgrade CTAs with correct variants
- Delegates to contact sales for Ultimate plan
The implementation properly uses
variant="outline"for downgrades and the default primary variant for upgrades, providing good visual hierarchy.Note: The
handlePlanChangehandler currently only logs to console and updates local state. Actual billing API integration appears to be deferred, which is appropriate for iterative development.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/desktop/src/components/main/sidebar/profile/index.tsx (3)
270-277: Pass an array tocnand group classNames per coding guidelines.As per coding guidelines.
Apply this diff:
- className={cn( - "flex w-full items-center gap-2.5", - "px-4 py-2", - "text-left", - "transition-all duration-300", - "hover:bg-neutral-100", - isExpanded && "bg-neutral-50 border-t border-neutral-100", - )} + className={cn([ + // Layout + "flex w-full items-center gap-2.5", + "px-4 py-2", + "text-left", + // Interactions + "transition-all duration-300", + "hover:bg-neutral-100", + isExpanded && "bg-neutral-50 border-t border-neutral-100", + ])}
281-288: Pass an array tocnand group classNames per coding guidelines.As per coding guidelines.
Apply this diff:
- className={cn( - "flex size-8 flex-shrink-0 items-center justify-center", - "overflow-hidden rounded-full", - "border border-white/60 border-t border-neutral-400", - "bg-gradient-to-br from-indigo-400 to-purple-500", - "shadow-sm", - "transition-transform duration-300", - )} + className={cn([ + // Layout & sizing + "flex size-8 flex-shrink-0 items-center justify-center", + "overflow-hidden rounded-full", + // Styling + "border border-white/60 border-t border-neutral-400", + "bg-gradient-to-br from-indigo-400 to-purple-500", + "shadow-sm", + // Interactions + "transition-transform duration-300", + ])}
322-327: Pass an array tocnper coding guidelines.As per coding guidelines.
Apply this diff:
<ChevronUpIcon - className={cn( - "h-4 w-4", - "transition-transform duration-300", - isExpanded ? "rotate-180 text-neutral-500" : "text-neutral-400", - )} + className={cn([ + // Sizing + "h-4 w-4", + // Interactions + "transition-transform duration-300", + isExpanded ? "rotate-180 text-neutral-500" : "text-neutral-400", + ])} />
♻️ Duplicate comments (4)
apps/desktop/src/components/settings/template/index.tsx (2)
14-14: Non-functional favorite filter state still present.The
FilterStatustype andfilterStatusstate remain defined but are not used in the filtering logic. This issue was previously flagged in past reviews.Also applies to: 19-19
49-67: Non-functional favorite filter buttons still present.The button group for filtering by "All" or "Favorite" remains in the UI but has no effect on the displayed templates. This creates a confusing user experience and was previously flagged for removal.
apps/desktop/src/components/main/sidebar/profile/index.tsx (2)
305-317: Improve className grouping for better readability.While the code now uses an array for
cn()(good!), the classNames could be better organized by logical sections as suggested in the previous review.Consider applying this grouping:
className={cn( [ + // Visibility & layout "hidden md:inline-block", - "cursor-pointer", "rounded-full", - "border border-neutral-900", - "bg-white", "px-2.5 py-0.5", + // Styling + "border border-neutral-900", + "bg-white", "text-[11px] font-medium text-neutral-900", - "transition-colors duration-200", + // Interactions + "cursor-pointer", "hover:bg-neutral-100", + "transition-colors duration-200", ], )}
104-111: Remove console.log and move closeMenu() into promise chain.Two issues remain:
- The
console.logshould be removed.- As flagged in the previous review,
closeMenu()executes synchronously whilewindowEmitNavigateis still pending. MovecloseMenu()into the.then()callback:Apply this diff:
const handleClickBilling = useCallback(() => { - console.log("handleClickBilling"); windowsCommands.windowShow({ type: "settings" }).then(() => { windowsCommands.windowEmitNavigate({ type: "settings" }, { path: "/app/settings", search: { tab: "billing" } }); + closeMenu(); }); - closeMenu(); }, [closeMenu]);
🧹 Nitpick comments (3)
apps/desktop/src/components/settings/template/index.tsx (1)
133-153: Consider extracting hook to separate file.The
useTemplateshook could be extracted toapps/desktop/src/hooks/useTemplates.tsxto follow the project's convention of keeping hooks in the hooks directory and improve reusability.apps/desktop/src/components/main/body/calendars.tsx (2)
254-276: Re-measurement may miss non-window layout changes.The
useEffectonly re-measures on window resize events. Layout changes triggered by other factors (e.g., sidebar toggle viasetSidebarOpen, flex container reflows) won't trigger re-measurement, potentially causing incorrectmaxVisibleItemscalculations.Consider using a
ResizeObserveron the cell element to detect any size changes, not just window resizes:useEffect(() => { + if (!cellRef.current) return; + + const resizeObserver = new ResizeObserver(() => { const measureHeight = () => { if (cellRef.current && contentRef.current) { const cellHeight = cellRef.current.clientHeight; const contentTop = contentRef.current.offsetTop; const availableHeight = cellHeight - contentTop; const EVENT_HEIGHT = 20; const SPACING = 4; const itemsWithSpacing = Math.floor((availableHeight + SPACING) / (EVENT_HEIGHT + SPACING)); setMaxVisibleItems(Math.max(1, itemsWithSpacing)); } }; + measureHeight(); + }); - measureHeight(); - window.addEventListener("resize", measureHeight); - return () => window.removeEventListener("resize", measureHeight); + resizeObserver.observe(cellRef.current); + return () => resizeObserver.disconnect(); }, []);
226-228: Potential layout shift on initial render.The initial
maxVisibleItemsstate of 5 may not match the actual available space, causing a brief layout shift when theuseEffectruns and updates the value after mount.To minimize layout shift, you could:
- Choose a more conservative initial value (e.g., 3) that's more likely to fit in most layouts
- Or, render with
visibility: hiddenuntil the first measurement completesThis is a minor UX concern and may not be noticeable in practice.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/desktop/src/components/main/body/calendars.tsx(8 hunks)apps/desktop/src/components/main/body/contacts/index.tsx(2 hunks)apps/desktop/src/components/main/body/events.tsx(2 hunks)apps/desktop/src/components/main/body/folders.tsx(4 hunks)apps/desktop/src/components/main/body/humans.tsx(2 hunks)apps/desktop/src/components/main/body/sessions/index.tsx(2 hunks)apps/desktop/src/components/main/body/shared.tsx(5 hunks)apps/desktop/src/components/main/sidebar/profile/index.tsx(10 hunks)apps/desktop/src/components/settings/billing.tsx(1 hunks)apps/desktop/src/components/settings/template/index.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/desktop/src/components/main/body/contacts/index.tsx
- apps/desktop/src/components/main/body/sessions/index.tsx
- apps/desktop/src/components/main/body/humans.tsx
- apps/desktop/src/components/main/body/events.tsx
- apps/desktop/src/components/settings/billing.tsx
🧰 Additional context used
📓 Path-based instructions (1)
apps/desktop/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (apps/desktop/.cursor/rules/style.mdc)
apps/desktop/**/*.{tsx,jsx}: When there are many Tailwind classNames with conditional logic, use the utilitycnimported asimport { cn } from "@hypr/utils"
When usingcnfor Tailwind classNames, always pass an array
Group Tailwind classNames by logical sections when usingcn(split array items by grouping)
Files:
apps/desktop/src/components/main/body/shared.tsxapps/desktop/src/components/settings/template/index.tsxapps/desktop/src/components/main/sidebar/profile/index.tsxapps/desktop/src/components/main/body/calendars.tsxapps/desktop/src/components/main/body/folders.tsx
🧠 Learnings (3)
📚 Learning: 2025-10-21T03:03:20.070Z
Learnt from: CR
PR: fastrepl/hyprnote#0
File: apps/desktop/.cursor/rules/style.mdc:0-0
Timestamp: 2025-10-21T03:03:20.070Z
Learning: Applies to apps/desktop/**/*.{tsx,jsx} : When there are many Tailwind classNames with conditional logic, use the utility `cn` imported as `import { cn } from "hypr/utils"`
Applied to files:
apps/desktop/src/components/main/body/shared.tsxapps/desktop/src/components/main/sidebar/profile/index.tsx
📚 Learning: 2025-10-21T03:03:20.070Z
Learnt from: CR
PR: fastrepl/hyprnote#0
File: apps/desktop/.cursor/rules/style.mdc:0-0
Timestamp: 2025-10-21T03:03:20.070Z
Learning: Applies to apps/desktop/**/*.{tsx,jsx} : Group Tailwind classNames by logical sections when using `cn` (split array items by grouping)
Applied to files:
apps/desktop/src/components/main/body/shared.tsxapps/desktop/src/components/main/sidebar/profile/index.tsx
📚 Learning: 2025-10-21T03:03:20.070Z
Learnt from: CR
PR: fastrepl/hyprnote#0
File: apps/desktop/.cursor/rules/style.mdc:0-0
Timestamp: 2025-10-21T03:03:20.070Z
Learning: Applies to apps/desktop/**/*.{tsx,jsx} : When using `cn` for Tailwind classNames, always pass an array
Applied to files:
apps/desktop/src/components/main/body/shared.tsxapps/desktop/src/components/main/sidebar/profile/index.tsx
🧬 Code graph analysis (3)
apps/desktop/src/components/main/body/shared.tsx (2)
apps/desktop/src/hooks/useCmdKeyPressed.tsx (1)
useCmdKeyPressed(7-40)packages/utils/src/cn.ts (1)
cn(20-22)
apps/desktop/src/components/settings/template/index.tsx (2)
packages/db/src/schema.ts (1)
templates(101-106)apps/desktop/src/components/settings/template/editor.tsx (1)
TemplateEditor(6-47)
apps/desktop/src/components/main/sidebar/profile/index.tsx (2)
apps/desktop/src/components/main/sidebar/profile/banner.tsx (1)
TryProBanner(7-70)packages/utils/src/cn.ts (1)
cn(20-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci (macos, macos-14)
🔇 Additional comments (11)
apps/desktop/src/components/settings/template/index.tsx (4)
22-38: Filtering logic correctly implemented.The search filtering is working correctly, and previous issues (no-op self-assignment, filterStatus in dependencies) have been properly addressed.
40-42: Clean internal state navigation.The conditional rendering of
TemplateEditorbased on internal state is well-implemented and avoids external navigation dependencies.
69-104: Search and template list well-implemented.The search functionality, empty state UI, and template list rendering are correctly implemented. The onClick handler properly integrates with the internal state navigation pattern.
107-131: TemplateCard follows coding guidelines.The component correctly uses the
cnutility with an array and logical grouping of Tailwind classes, as per the coding guidelines.As per coding guidelines
apps/desktop/src/components/main/sidebar/profile/index.tsx (1)
203-232: LGTM! Exit animation issue resolved.The banner animation issue from the previous review has been correctly addressed.
TryProBannernow stays mounted when not authenticated and receives theisDismissedprop, allowing the exit animation to play properly before the Sign in button appears.apps/desktop/src/components/main/body/folders.tsx (1)
25-25: LGTM! ConsistenttabIndexprop threading.The
tabIndexprop is properly destructured and forwarded toTabItemBasein both folder variants, aligning with the broader keyboard navigation enhancement across all tab types.Also applies to: 37-37, 48-48, 63-63
apps/desktop/src/components/main/body/shared.tsx (3)
3-4: LGTM! Propercnutility usage per coding guidelines.The code now correctly uses
cnfrom@hypr/utilsinstead ofclsx, passes arrays, and groups Tailwind classes logically. This addresses the previous review feedback.As per coding guidelines.
Also applies to: 66-71, 82-87
8-8: LGTM! Clean keyboard shortcut implementation.The conditional shortcut badge rendering is well-implemented:
- Uses the
useCmdKeyPressedhook to detect Cmd key state- Shows
⌘+tabIndexbadge only when Cmd is pressed andtabIndexis defined- Positioned at top-right with
pointer-events-noneto avoid interfering with interactionsAlso applies to: 40-40, 58-58, 93-100
12-12: LGTM! Proper type extension for keyboard navigation.The
tabIndex?: numberprop is correctly added to bothTabItemPropsandTabItemBaseProps, maintaining type safety across the tab component hierarchy.Also applies to: 19-19, 33-33
apps/desktop/src/components/main/body/calendars.tsx (2)
29-29: LGTM! ConsistenttabIndexprop threading.The
tabIndexprop is properly destructured and forwarded toTabItemBase, consistent with other tab components.Also applies to: 41-41
143-143: LGTM! Button size adjustment.The "Today" button now uses
size="sm"for a more compact appearance, consistent with UI refinements.
| // Measure actual available height and calculate max visible items | ||
| useEffect(() => { | ||
| const measureHeight = () => { | ||
| if (cellRef.current && contentRef.current) { | ||
| const cellHeight = cellRef.current.clientHeight; | ||
| const contentTop = contentRef.current.offsetTop; | ||
| const availableHeight = cellHeight - contentTop; | ||
| const EVENT_HEIGHT = 20; // height of each event item (h-5) | ||
| const SPACING = 4; // space-y-1 | ||
|
|
||
| // Calculate how many items can fit | ||
| const itemsWithSpacing = Math.floor((availableHeight + SPACING) / (EVENT_HEIGHT + SPACING)); | ||
| // Reserve space for "+x more" if needed | ||
| setMaxVisibleItems(Math.max(1, itemsWithSpacing)); | ||
| } | ||
| }; | ||
|
|
||
| measureHeight(); | ||
|
|
||
| // Re-measure on window resize | ||
| window.addEventListener("resize", measureHeight); | ||
| return () => window.removeEventListener("resize", measureHeight); | ||
| }, []); |
There was a problem hiding this comment.
Magic numbers hardcoded in height calculation.
The EVENT_HEIGHT (20) and SPACING (4) constants are hardcoded within the useEffect and tightly coupled to CSS classes (h-5, space-y-1). If the CSS changes, this calculation will silently break.
Consider extracting these as constants at the component or module level with clear documentation linking them to the corresponding CSS classes:
+// Must match h-5 class (1.25rem = 20px at default scale)
+const EVENT_HEIGHT = 20;
+// Must match space-y-1 class (0.25rem = 4px at default scale)
+const EVENT_SPACING = 4;
+
function TabContentCalendarDay({
day,
isCurrentMonth,Then reference these constants in the useEffect. Even better: compute these values from the actual rendered DOM elements instead of hardcoding.
Committable suggestion skipped: line range outside the PR's diff.
| // Mock auth state - toggle this to test different states | ||
| const [isAuthenticated, setIsAuthenticated] = useState(false); | ||
| const [isBannerDismissed, setIsBannerDismissed] = useState(false); |
There was a problem hiding this comment.
Replace mock authentication with real implementation.
The hardcoded mock authentication state should be replaced with actual authentication logic before merging to production.
🤖 Prompt for AI Agents
In apps/desktop/src/components/main/sidebar/profile/index.tsx around lines
26-28, replace the hardcoded mock auth state with the real authentication
source: remove useState(false) for isAuthenticated and instead consume the app's
auth provider/hook (e.g., useAuth() or AuthContext) to derive current
user/authenticated boolean; wire setIsBannerDismissed to persistent/local state
(or context) if required; ensure you subscribe to auth changes (or use the
hook's reactive value) and remove any leftover mock toggles, and add any
necessary cleanup or null checks for user before rendering profile UI.
| const handleAuth = useCallback(() => { | ||
| if (isAuthenticated) { | ||
| // Mock logout | ||
| console.log("Logging out..."); | ||
| setIsAuthenticated(false); | ||
| setIsBannerDismissed(false); | ||
| } else { | ||
| // Mock sign in | ||
| console.log("Signing in..."); | ||
| setIsAuthenticated(true); | ||
| } | ||
| closeMenu(); | ||
| }, [isAuthenticated, closeMenu]); |
There was a problem hiding this comment.
Remove debug logs and implement real authentication.
The console.log statements and mock authentication logic need to be replaced with actual sign-in/sign-out implementation.
🤖 Prompt for AI Agents
In apps/desktop/src/components/main/sidebar/profile/index.tsx around lines 38 to
50, remove the console.log debug lines and the mock sign-in/out toggles and
replace them with real authentication calls: call your auth service or hook
(e.g., auth.signIn()/auth.signOut() or useAuth().signIn/signOut), await the call
inside a try/catch, update local state (setIsAuthenticated and
setIsBannerDismissed) based on the real auth response, handle and log errors via
the app logger (not console.log), keep closeMenu() after successful auth, and
add the auth functions to the useCallback dependency list.
| const handleClickProfile = useCallback(() => { | ||
| // TODO: Show the user's own profile in the contacts view | ||
| openNew({ | ||
| type: "contacts", | ||
| state: { | ||
| selectedOrganization: null, | ||
| selectedPerson: null, | ||
| }, | ||
| }); | ||
| closeMenu(); | ||
| }, [openNew, closeMenu]); |
There was a problem hiding this comment.
Complete the My Profile implementation.
The TODO indicates this should show the user's own profile, but currently it opens an empty contacts view identical to the Contacts menu item.
Do you want me to help design a proper implementation that displays the authenticated user's profile, or open an issue to track this task?
🤖 Prompt for AI Agents
In apps/desktop/src/components/main/sidebar/profile/index.tsx around lines
142–152, handleClickProfile currently opens the contacts view with null
selections; instead fetch the authenticated user's identity (via the existing
auth/context selector or a useCurrentUser hook), then call openNew with type
"contacts" and state.selectedPerson set to the current user's person ID (and
state.selectedOrganization to the user's org ID if available). Add the user
dependency to the useCallback dependency array and handle the no-user case by
either opening the generic contacts view or redirecting to login/account
settings.
|
|
||
| const USER_TEMPLATE_QUERY = "user_templates"; | ||
|
|
||
| const quries = persisted.UI.useCreateQueries( |
There was a problem hiding this comment.
Fix typo in variable name.
The variable is named quries but should be queries. This typo appears on lines 139 and 151.
Apply this diff to fix the typo:
- const quries = persisted.UI.useCreateQueries(
+ const queries = persisted.UI.useCreateQueries(
store,
(store) =>
createQueries(store).setQueryDefinition(USER_TEMPLATE_QUERY, "templates", ({ select, where }) => {
select("title");
select("description");
select("sections");
where("user_id", user_id ?? "");
}),
[user_id],
);
- const templates = persisted.UI.useResultTable(USER_TEMPLATE_QUERY, quries);
+ const templates = persisted.UI.useResultTable(USER_TEMPLATE_QUERY, queries);Also applies to: 151-151
🤖 Prompt for AI Agents
In apps/desktop/src/components/settings/template/index.tsx around lines 139 and
151, the local variable is misspelled as `quries`; rename it to `queries` in
both occurrences and update any references in this file to use the corrected
identifier so imports/uses remain consistent and TypeScript/ESLint errors are
resolved.
| ); | ||
|
|
||
| const templates = persisted.UI.useResultTable(USER_TEMPLATE_QUERY, quries); | ||
| return templates as unknown as Record<string, persisted.Template>; |
There was a problem hiding this comment.
Avoid double type casting; fix type mismatch.
The as unknown as pattern bypasses TypeScript's type safety and indicates a type mismatch between useResultTable's return type and persisted.Template. This could lead to runtime errors if the types don't actually match.
Either fix the underlying type definitions or add runtime validation to ensure type safety.
🤖 Prompt for AI Agents
In apps/desktop/src/components/settings/template/index.tsx around line 152,
remove the unsafe "as unknown as Record<string, persisted.Template>" cast and
fix the underlying mismatch: either update the useResultTable return type to
accurately reflect Record<string, persisted.Template>, or map/transform the
returned templates into a new object that conforms to persisted.Template
(validating required fields and converting types where necessary) and then
return that safely typed object; if runtime shapes may vary, add explicit
runtime checks (throw or log and skip invalid entries) before returning to
preserve type safety.
2 participants
• Added banner component
• Implemented sign-in functionality
• Added logout feature
• Adjusted calendar height