Conversation
## Description Improved error handling in the ZeroAgent class by providing more specific error messages when authentication fails. The changes include: 1. Enhanced the error message in the execute method to specify whether the failure is due to missing driver or connectionId 2. Improved the error message in onConnect method to clarify when a token is missing 3. Removed redundant driver check in onChatMessage method as this validation is already handled elsewhere 4. Cleaned up imports by removing unused WSMessage type ## Type of Change - [x] 🐛 Bug fix (non-breaking change which fixes an issue) - [x] 🔒 Security enhancement ## Areas Affected - [x] Authentication/Authorization - [x] API Endpoints ## Testing Done - [x] Manual testing performed ## Security Considerations - [x] Authentication checks are in place ## Checklist - [x] I have performed a self-review of my code - [x] My changes generate no new warnings _By submitting this pull request, I confirm that my contribution is made under the terms of the project's license._
done: - adds new hook `useOptimisticAction` - in mail-lsit - in thread-display - adds undo action to toast (design needs updating) wip: - undo sent email within 10-20 seconds - update toast design/undo button - add shortcut to undo 😎 - safari causing slight issues but it works
# READ CAREFULLY THEN REMOVE Remove bullet points that are not relevant. PLEASE REFRAIN FROM USING AI TO WRITE YOUR CODE AND PR DESCRIPTION. IF YOU DO USE AI TO WRITE YOUR CODE PLEASE PROVIDE A DESCRIPTION AND REVIEW IT CAREFULLY. MAKE SURE YOU UNDERSTAND THE CODE YOU ARE SUBMITTING USING AI. - Pull requests that do not follow these guidelines will be closed without review or comment. - If you use AI to write your PR description your pr will be close without review or comment. - If you are unsure about anything, feel free to ask for clarification. ## Description Please provide a clear description of your changes. --- ## Type of Change Please delete options that are not relevant. - [ ] 🐛 Bug fix (non-breaking change which fixes an issue) - [ ] ✨ New feature (non-breaking change which adds functionality) - [ ] 💥 Breaking change (fix or feature with breaking changes) - [ ] 📝 Documentation update - [ ] 🎨 UI/UX improvement - [ ] 🔒 Security enhancement - [ ] ⚡ Performance improvement ## Areas Affected Please check all that apply: - [ ] Email Integration (Gmail, IMAP, etc.) - [ ] User Interface/Experience - [ ] Authentication/Authorization - [ ] Data Storage/Management - [ ] API Endpoints - [ ] Documentation - [ ] Testing Infrastructure - [ ] Development Workflow - [ ] Deployment/Infrastructure ## Testing Done Describe the tests you've done: - [ ] Unit tests added/updated - [ ] Integration tests added/updated - [ ] Manual testing performed - [ ] Cross-browser testing (if UI changes) - [ ] Mobile responsiveness verified (if UI changes) ## Security Considerations For changes involving data or authentication: - [ ] No sensitive data is exposed - [ ] Authentication checks are in place - [ ] Input validation is implemented - [ ] Rate limiting is considered (if applicable) ## Checklist - [ ] I have read the [CONTRIBUTING](../CONTRIBUTING.md) document - [ ] My code follows the project's style guidelines - [ ] I have performed a self-review of my code - [ ] I have commented my code, particularly in complex areas - [ ] I have updated the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix/feature works - [ ] All tests pass locally - [ ] Any dependent changes are merged and published ## Additional Notes Add any other context about the pull request here. ## Screenshots/Recordings Add screenshots or recordings here if applicable. --- _By submitting this pull request, I confirm that my contribution is made under the terms of the project's license._
# Improve Thread Context Menu and Optimistic Updates This PR refactors the thread context menu and optimistic state handling in the mail application to fix several issues: 1. Simplifies the `ThreadContextMenu` component by removing the redundant `emailId` prop and only using `threadId` 2. Adds support for optimistic marking as unread in the thread context menu 3. Improves the optimistic state handling in the mail list by: - Using a consistent thread ID across components - Properly displaying optimistic read/unread state - Fixing star toggling functionality 4. Enhances the optimistic actions system to invalidate specific queries rather than refetching everything ## Type of Change - [x] 🐛 Bug fix (non-breaking change which fixes an issue) - [x] ⚡ Performance improvement ## Areas Affected - [x] User Interface/Experience - [x] Data Storage/Management ## Testing Done - [x] Manual testing performed ## Checklist - [x] I have performed a self-review of my code - [x] My changes generate no new warnings _By submitting this pull request, I confirm that my contribution is made under the terms of the project's license._
# READ CAREFULLY THEN REMOVE Remove bullet points that are not relevant. PLEASE REFRAIN FROM USING AI TO WRITE YOUR CODE AND PR DESCRIPTION. IF YOU DO USE AI TO WRITE YOUR CODE PLEASE PROVIDE A DESCRIPTION AND REVIEW IT CAREFULLY. MAKE SURE YOU UNDERSTAND THE CODE YOU ARE SUBMITTING USING AI. - Pull requests that do not follow these guidelines will be closed without review or comment. - If you use AI to write your PR description your pr will be close without review or comment. - If you are unsure about anything, feel free to ask for clarification. ## Description Please provide a clear description of your changes. --- ## Type of Change Please delete options that are not relevant. - [ ] 🐛 Bug fix (non-breaking change which fixes an issue) - [ ] ✨ New feature (non-breaking change which adds functionality) - [ ] 💥 Breaking change (fix or feature with breaking changes) - [ ] 📝 Documentation update - [ ] 🎨 UI/UX improvement - [ ] 🔒 Security enhancement - [ ] ⚡ Performance improvement ## Areas Affected Please check all that apply: - [ ] Email Integration (Gmail, IMAP, etc.) - [ ] User Interface/Experience - [ ] Authentication/Authorization - [ ] Data Storage/Management - [ ] API Endpoints - [ ] Documentation - [ ] Testing Infrastructure - [ ] Development Workflow - [ ] Deployment/Infrastructure ## Testing Done Describe the tests you've done: - [ ] Unit tests added/updated - [ ] Integration tests added/updated - [ ] Manual testing performed - [ ] Cross-browser testing (if UI changes) - [ ] Mobile responsiveness verified (if UI changes) ## Security Considerations For changes involving data or authentication: - [ ] No sensitive data is exposed - [ ] Authentication checks are in place - [ ] Input validation is implemented - [ ] Rate limiting is considered (if applicable) ## Checklist - [ ] I have read the [CONTRIBUTING](../CONTRIBUTING.md) document - [ ] My code follows the project's style guidelines - [ ] I have performed a self-review of my code - [ ] I have commented my code, particularly in complex areas - [ ] I have updated the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix/feature works - [ ] All tests pass locally - [ ] Any dependent changes are merged and published ## Additional Notes Add any other context about the pull request here. ## Screenshots/Recordings Add screenshots or recordings here if applicable. --- _By submitting this pull request, I confirm that my contribution is made under the terms of the project's license._
## Description Added support for optimistic toggling of "Important" flag in emails. This enhancement allows users to mark emails as important with immediate UI feedback while the action processes in the background. The implementation follows the same pattern as other optimistic actions like starring and marking as read. Also fixed a few UI issues including removing extra whitespace in the AI chat component and removing debug mode from the Party provider. ## Type of Change - [x] ✨ New feature (non-breaking change which adds functionality) - [x] 🐛 Bug fix (non-breaking change which fixes an issue) - [x] 🎨 UI/UX improvement ## Areas Affected - [x] User Interface/Experience - [x] Email Integration (Gmail, IMAP, etc.) ## Testing Done - [x] Manual testing performed ## Checklist - [x] I have performed a self-review of my code - [x] My changes generate no new warnings - [x] My code follows the project's style guidelines ## Additional Notes The implementation includes: 1. Adding the Important button to the thread list UI 2. Supporting optimistic state updates for the Important flag 3. Fixing the ZeroAgent to use user ID instead of email for better identification 4. Removing debug mode from Party provider to reduce console noise
### TL;DR Improved mail list hotkey functionality by adding a `shouldUseHover` flag to determine when to use hover actions versus bulk selection actions. ### What changed? - Added a `shouldUseHover` flag that checks if bulk selection is empty - Updated hotkey handlers (markAsRead, markAsUnread, archiveEmail) to use this flag when determining whether to act on hovered email or selected emails - Removed unused imports and functions: - Removed `useQueryClient`, `useTRPC`, `useStats` imports - Removed `refetch`, `mutateStats`, `invalidateCount` functions - Removed a console.log statement in the selectAll function - Updated dependency arrays in useCallback hooks to include the new `shouldUseHover` flag ### How to test? 1. Test mail list hotkeys with no emails selected: - Hover over an email and use keyboard shortcuts to mark as read/unread or archive - Verify actions apply only to the hovered email 2. Test mail list hotkeys with emails selected: - Select multiple emails and use keyboard shortcuts - Verify actions apply to all selected emails regardless of hover state ### Why make this change? This change improves the user experience by making the hotkey behavior more predictable and intuitive. When users have explicitly selected emails, actions should apply to those selections. When no emails are selected, actions should apply to the currently hovered email. This creates a more consistent interaction model and prevents accidental operations on unintended emails.
|
""" WalkthroughThis update introduces a comprehensive theming system for the mail application. It adds database schema, server-side logic, and TRPC endpoints for user and public themes, as well as client-side context, hooks, utilities, and UI for theme creation, selection, and management. Numerous styling and background color variables are refactored for dynamic theming. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (ThemesPage/ThemeCreator)
participant ThemeContext
participant Theme Hooks
participant TRPC ThemesRouter
participant DB (ThemeManager)
User->>UI (ThemesPage): Selects or creates a theme
UI (ThemesPage)->>Theme Hooks: Fetches user/public themes
Theme Hooks->>TRPC ThemesRouter: list/marketplace/getPublic
TRPC ThemesRouter->>DB (ThemeManager): Query themes
DB (ThemeManager)-->>TRPC ThemesRouter: Return theme data
TRPC ThemesRouter-->>Theme Hooks: Return theme data
Theme Hooks-->>UI (ThemesPage): Provide themes
User->>UI (ThemeCreator): Customizes and saves a theme
UI (ThemeCreator)->>Theme Hooks: createTheme/updateTheme
Theme Hooks->>TRPC ThemesRouter: create/update
TRPC ThemesRouter->>DB (ThemeManager): Insert/update theme
DB (ThemeManager)-->>TRPC ThemesRouter: Confirm operation
TRPC ThemesRouter-->>Theme Hooks: Success/failure
Theme Hooks-->>UI (ThemeCreator): Show toast/result
User->>UI (ThemesPage): Applies a theme
UI (ThemesPage)->>ThemeContext: applyTheme/revertToDefault
ThemeContext->>DOM: Injects CSS, toggles dark mode
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🔭 Outside diff range comments (6)
apps/mail/lib/hotkeys/mail-list-hotkeys.tsx (1)
102-109:idsToMarkis now undefined – breaks ‘mark as important’You renamed the local variable to
idsToArchivebut the mutator still references the old name. This throws at runtime.- const idsToArchive = mail.bulkSelected; - if (idsToArchive.length === 0) { + const idsToMark = mail.bulkSelected; + if (idsToMark.length === 0) { toast.info(t('common.mail.noEmailsToSelect')); return; } - optimisticToggleImportant(idsToMark, true); + optimisticToggleImportant(idsToMark, true);(Alternatively keep the new variable name and change the call site – just make them consistent.)
Add a quick unit test for this handler; it’s easy to miss in manual QA.
apps/server/src/db/migrations/meta/_journal.json (1)
202-214: Duplicate migration index (idx = 28) will break migration orderingTwo entries share
idx: 28(0028_worried_molecule_manand0028_soft_blazing_skull).
Most migration runners expect a strictly monotonic index and will either abort or apply in undefined order.Increment the second entry to 29 (and subsequent snapshots/scripts) or re-sequence the file.
apps/mail/providers/client-providers.tsx (1)
9-15: Remove leftoverThemeProviderimport & unusedthemevariable
ThemeProvideris no longer rendered but is still imported; likewise the computedthemevalue is never used. Most TS/ESLint configs will fail withno-unused-vars/no-unused-imports, breaking the build.-import { ThemeProvider } from 'next-themes'; ... - const theme = data?.settings.colorTheme || 'system';If you still need the raw
colorThemeelsewhere, keep the variable and pass it to the custom provider; otherwise delete both lines.apps/mail/components/mail/thread-display.tsx (1)
719-737: Hard-coded hex colours bypass new theme systemThe container now uses
bg-[#ffffff]/dark:bg-[#1a1a1a]and several child nodes use explicit hex text colours.
These override the CSS variables injected byThemeContextProvider, meaning custom themes won’t affect this view.Replace with semantic classes (
bg-background,bg-card,text-muted-foreground, etc.) or CSS vars:- '… rounded-xl bg-[#ffffff] transition-all duration-300 dark:bg-[#1a1a1a]', + '… rounded-xl bg-background transition-all duration-300 dark:bg-background',Same for the
dark:text-[#959595]literals below.Failing to align with the new token system undermines the very feature this PR adds.
apps/mail/app/(routes)/settings/appearance/page.tsx (2)
2-12: Remove unused imports to avoid dead code & bundle bloatAll imported symbols here are no longer referenced after the refactor. They will trigger linter errors and inflate the bundle size.
-import { useMutation, useQuery } from '@tanstack/react-query'; -import ThemesPage from '@/components/theme/theme-view'; -import { zodResolver } from '@hookform/resolvers/zod'; -import { useTRPC } from '@/providers/query-provider'; -import { useSettings } from '@/hooks/use-settings'; -import { useTranslations } from 'use-intl'; -import { useForm } from 'react-hook-form'; -import { useTheme } from 'next-themes'; -import { useState } from 'react'; -import { toast } from 'sonner'; -import * as z from 'zod'; +import { useTranslations } from 'use-intl'; +import ThemesPage from '@/components/theme/theme-view';
14-18: Delete leftover form schema / type definitions
formSchemaand theThemealias are now unused. Keeping them around confuses future readers.-const formSchema = z.object({ - colorTheme: z.enum(['dark', 'light', 'system', '']), -}); - -type Theme = 'dark' | 'light' | 'system';
🧹 Nitpick comments (14)
.gitignore (2)
61-61: Confirm/update-checkignore pattern.Ensure that
/update-checkis indeed a generated artifact from your devcontainer or build process and not a directory users need to commit. Consider adding a comment above this entry to clarify its origin and purpose.
63-63: Validate ignoringtools.json.Verify that
tools.jsonis always auto-generated and not part of necessary configuration (e.g., workspace or CI settings). If it’s intentionally ephemeral, this entry is appropriate; otherwise document or remove it..env.example (1)
36-37: Keep env keys ordered and ensure trailing newlineLinters flag
ELEVENLABS_API_KEYbecause:
- Keys are expected to stay alphabetically grouped (it should sit above
TWILIO_ACCOUNT_SID).- The file is missing a terminating blank line.
-TWILIO_PHONE_NUMBER= -ELEVENLABS_API_KEY= +ELEVENLABS_API_KEY= +TWILIO_PHONE_NUMBER= +apps/mail/components/mail/mail-iframe.tsx (1)
95-99: No-op blank line – consider removing to avoid noisy diffsThe additional blank line adds churn without functional gain. Trimming it keeps history cleaner.
apps/mail/components/magicui/file-tree.tsx (1)
234-235: Class string no longer needs a template literalSince there’s no interpolation, the back-ticked string could be a plain string, or you could inline into
cndirectly. Not critical but removes unnecessary template overhead.- `hover:bg-accent/50 flex items-center gap-1 rounded-lg px-2 py-1.5 text-sm`, + 'hover:bg-accent/50 flex items-center gap-1 rounded-lg px-2 py-1.5 text-sm',apps/mail/components/ui/slider.tsx (1)
12-25: Vertical orientation styles missing
@radix-ui/react-sliderexposesdata-orientation="vertical"whenorientation="vertical".
Without explicit height / width tweaks, the track & thumb look broken in vertical mode.- <SliderPrimitive.Track className="relative h-1.5 w-full …"> + <SliderPrimitive.Track className="relative data-[orientation=horizontal]:h-1.5 + data-[orientation=horizontal]:w-full + data-[orientation=vertical]:h-full + data-[orientation=vertical]:w-1.5 + …">Same goes for
Thumbtransform offsets.
Fine if your app never uses vertical sliders, but worth guarding for future reuse.apps/mail/hooks/use-themes.ts (1)
1-246: Well-implemented theme hooks with proper React Query patterns.The hooks are well-structured with appropriate cache invalidation, error handling, and TypeScript typing. The infinite query pattern for marketplace themes is correctly implemented.
Consider using more explicit boolean checks instead of double negation for clarity:
- enabled: !!session?.user.id, + enabled: session?.user.id !== undefined,- enabled: !!themeId && !!session?.user.id, + enabled: themeId !== undefined && session?.user.id !== undefined,- enabled: !!themeId, + enabled: themeId !== undefined && themeId !== null,apps/server/src/lib/schemas.ts (1)
75-107: Validate color strings more strictly
themeColorsSchemacurrently accepts any string for each color value. Allowing arbitrary strings will make it easy for malformed values (e.g."rgb( )"or"foo") to slip through, break CSS, and crash the customiser.
Consider constraining each entry with a basic regexp such as/^\d+(\.\d+)? \d+% \d+%$/(or a dedicated helper) so only valid H S L tokens are persisted.apps/mail/types/index.ts (1)
163-168: Missing semicolon & lint violationA semicolon after
css: stringkeeps the style consistent with the rest of the file and avoids lint noise.- css: string + css: string;apps/mail/components/theme/theme-customizer.tsx (2)
261-313:NodeJS.Timeouttype leaks Node typings into the browserThe closure typed as
NodeJS.Timeoutpulls in the Node DOM-shim and may break builds configured withdomonly libs.
UseReturnType<typeof setTimeout>(or simplynumber) instead.
389-400: Event listener leak
handleClickOutsideis registered ondocumentbut never removed on component unmount.
Wrap the cleanup in the returned function ofuseEffect:useEffect(() => { const handleClickOutside = (event: MouseEvent) => { … }; document.addEventListener('mousedown', handleClickOutside); return () => { document.removeEventListener('mousedown', handleClickOutside); }; }, []);apps/server/src/lib/theme-manager.ts (1)
86-92: Redundant equality checks slow public-theme searchInside the
searchQuerybranch you already uselike()for partial matches; the twoeq()clauses on the same fields add no value and force the planner to evaluate extra OR predicates.- eq(theme.name, searchQuery), - eq(theme.description, searchQuery), like(theme.name, `%${searchQuery}%`), like(theme.description, `%${searchQuery}%`),Removing the
eqterms simplifies the SQL without changing behaviour.apps/mail/components/context/theme-context.tsx (1)
147-191: Left-over console logs will pollute production consoleAll
console.log/console.warnstatements insideapplyTheme,revertToDefault, etc. should be removed or gated behindprocess.env.NODE_ENV !== 'production'checks.apps/mail/lib/themes/theme-utils.ts (1)
373-391: Remove or export unusedinvertColorfunction.The
invertColorfunction is defined but never used in this file. Either remove it or export it if it's intended for use elsewhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (33)
.env.example(1 hunks).gitignore(1 hunks)apps/mail/app/(routes)/settings/appearance/page.tsx(2 hunks)apps/mail/app/globals.css(12 hunks)apps/mail/app/routes.ts(1 hunks)apps/mail/components/context/theme-context.tsx(1 hunks)apps/mail/components/magicui/file-tree.tsx(1 hunks)apps/mail/components/mail/mail-display.tsx(3 hunks)apps/mail/components/mail/mail-iframe.tsx(2 hunks)apps/mail/components/mail/mail.tsx(6 hunks)apps/mail/components/mail/thread-display.tsx(3 hunks)apps/mail/components/theme/theme-customizer.tsx(1 hunks)apps/mail/components/theme/theme-switcher.tsx(0 hunks)apps/mail/components/theme/theme-view.tsx(1 hunks)apps/mail/components/ui/nav-main.tsx(2 hunks)apps/mail/components/ui/settings-content.tsx(1 hunks)apps/mail/components/ui/slider.tsx(1 hunks)apps/mail/hooks/use-themes.ts(1 hunks)apps/mail/lib/hotkeys/mail-list-hotkeys.tsx(2 hunks)apps/mail/lib/themes/index.tsx(1 hunks)apps/mail/lib/themes/theme-utils.ts(1 hunks)apps/mail/package.json(3 hunks)apps/mail/providers/client-providers.tsx(2 hunks)apps/mail/tailwind.config.ts(2 hunks)apps/mail/types/index.ts(1 hunks)apps/server/src/db/migrations/0028_soft_blazing_skull.sql(1 hunks)apps/server/src/db/migrations/meta/0028_snapshot.json(13 hunks)apps/server/src/db/migrations/meta/_journal.json(1 hunks)apps/server/src/db/schema.ts(2 hunks)apps/server/src/lib/schemas.ts(2 hunks)apps/server/src/lib/theme-manager.ts(1 hunks)apps/server/src/trpc/index.ts(2 hunks)apps/server/src/trpc/routes/themes.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/mail/components/theme/theme-switcher.tsx
🧰 Additional context used
🧬 Code Graph Analysis (9)
apps/server/src/trpc/index.ts (1)
apps/server/src/trpc/routes/themes.ts (1)
themesRouter(18-181)
apps/mail/lib/hotkeys/mail-list-hotkeys.tsx (1)
apps/mail/hooks/use-threads.ts (1)
useThreads(12-59)
apps/mail/app/(routes)/settings/appearance/page.tsx (1)
apps/mail/components/theme/theme-view.tsx (1)
ThemesPage(54-440)
apps/mail/components/ui/slider.tsx (1)
apps/mail/lib/utils.ts (1)
cn(51-51)
apps/mail/types/index.ts (1)
apps/server/src/lib/schemas.ts (1)
ThemeData(125-125)
apps/server/src/lib/schemas.ts (1)
apps/mail/types/index.ts (1)
ThemeData(170-179)
apps/server/src/lib/theme-manager.ts (4)
apps/server/src/db/index.ts (1)
DB(11-11)apps/server/src/lib/schemas.ts (4)
Theme(3-13)ThemeData(125-125)CreateTheme(126-126)UpdateTheme(127-127)apps/server/src/db/schema.ts (1)
theme(177-189)apps/mail/types/index.ts (1)
ThemeData(170-179)
apps/mail/components/mail/mail-display.tsx (1)
apps/mail/components/icons/icons.tsx (2)
Lightning(774-789)Users(510-540)
apps/mail/lib/themes/theme-utils.ts (4)
apps/mail/types/index.ts (4)
ThemeColors(145-161)ApiThemeResponse(163-168)ThemeData(170-179)CustomTheme(181-191)apps/mail/components/theme/theme-customizer.tsx (1)
hslToHex(123-182)apps/server/src/lib/schemas.ts (3)
ThemeColorSchema(128-128)ThemeData(125-125)Theme(3-13)apps/server/src/db/schema.ts (1)
theme(177-189)
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 37-37: [EndingBlankLine] No blank line at the end of the file
[warning] 37-37: [UnorderedKey] The ELEVENLABS_API_KEY key should go before the TWILIO_ACCOUNT_SID key
🪛 Biome (1.9.4)
apps/mail/lib/hotkeys/mail-list-hotkeys.tsx
[error] 18-18: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
apps/mail/components/theme/theme-view.tsx
[error] 233-359: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
apps/mail/lib/themes/theme-utils.ts
[error] 132-132: background is assigned to itself.
This is where is assigned.
(lint/correctness/noSelfAssign)
[error] 303-303: background is assigned to itself.
This is where is assigned.
(lint/correctness/noSelfAssign)
🔇 Additional comments (19)
apps/mail/app/routes.ts (1)
51-53: Commented-out route may orphan the create-theme page
/settings/appearance/createis now unreachable yet its page component still exists. If the new theming UX no longer needs a dedicated route, delete the file (and related imports) to avoid dead code; otherwise re-enable the route.apps/mail/components/ui/settings-content.tsx (1)
7-7: Background class update looks goodSwitching to the
bg-sidebartoken keeps the component in sync with the new CSS-variable driven theming system. No issues spotted.apps/server/src/trpc/index.ts (1)
7-11: Router wiring OK – double-check voice route deactivation
themesRouteris correctly added toappRouter. Confirm that commenting outvoiceRouteris intentional and that no client code still calls/voiceendpoints; otherwise those requests will now 404.apps/mail/package.json (1)
26-37: New UI dependencies – verify bundle & licence impact
@radix-ui/react-slider,@tabler/icons-react,@tanstack/react-table, andreact-colorare heavy packages.
Please confirm:
- Tree-shaking keeps the client bundle delta acceptable.
- Licences (Radix MIT, Tabler MIT, react-color MIT) are compatible with project policy.
- No duplication with existing colour pickers (
react-colorfulis already present).Also applies to: 93-94, 128-129
apps/mail/tailwind.config.ts (2)
20-25:darkBackgroundandlightBackgroundnow point to the same CSS variableBoth colours resolve to
hsl(var(--sidebar)), making the distinction between dark and light moot.
If the intention was a dynamic variable that changes per theme, consider renaming to a singlesidebarBackgroundtoken or mapping to different variables (--sidebar-dark/--sidebar-light) to avoid confusion.
75-76: Variable rename aligns with CSS change – good catchSwitching
--sidebar-backgroundto--sidebarkeeps the Tailwind theme in sync withglobals.css.apps/mail/providers/client-providers.tsx (1)
19-27: Consider passing the user’s preferred theme intoThemeContextProviderPreviously
ThemeProviderconsumed thethemeprop so user preference persisted. After the swap the preference isn’t forwarded, so the context may fall back to its default (usually “system”). If the new provider accepts an initial theme, wire it here:<ThemeContextProvider initialTheme={data?.settings.colorTheme}> … </ThemeContextProvider>apps/mail/components/mail/mail-display.tsx (1)
334-338: Tailwind classfill-primary-foregroundnot standard
fill-primary-foregroundisn’t generated by default Tailwind presets.
If you rely on the shadcn-ui palette it should befill-primary-foreground/??.
Otherwise, extend the safelist or replace withtext-primary-foreground+fill-current.Verify it compiles; otherwise icon fill will fall back to
currentColor.apps/server/src/db/migrations/meta/0028_snapshot.json (1)
1-835: Snapshot change looks goodChanges are formatting-only; data integrity and constraints remain unchanged.
apps/mail/components/mail/mail.tsx (4)
562-562: Minor styling adjustment looks good.The button styling change is appropriate and maintains consistency.
608-609: Good use of CSS variables for theming.The change from hardcoded colors to CSS variables (
bg-[#ffffff]anddark:bg-[#1a1a1a]) enables proper theme support.
623-623: Consistent theming for mobile view.Good consistency with the desktop thread panel styling.
919-919: Excellent use of CSS variable for theme support.Replacing the hardcoded color
#F59E0Dwithbg-primaryenables proper theme customization for the important category.apps/mail/components/ui/nav-main.tsx (1)
300-335: Excellent refactoring to declarative routing.The migration from imperative navigation to React Router's
Linkcomponent is well-executed. All functionality is preserved including click handlers, target attributes, and mobile sidebar behavior.apps/server/src/trpc/routes/themes.ts (1)
1-181: Well-implemented TRPC router with proper security controls.The theme router implementation is excellent:
- Proper rate limiting on mutation endpoints
- Good separation between public and private procedures
- Comprehensive input validation with Zod schemas
- Clean middleware pattern for ThemeManager injection
- Appropriate rate limits for different operations
apps/server/src/lib/theme-manager.ts (1)
153-180:updateThemeallows empty payload – unintended no-op
createThemeSchema.partial()makes every field optional;updateThemethen forwards the untouchedupdateDatastraight to the DB.
Calling the endpoint with{ id }and nothing else silently updates onlyupdatedAt, which is surprising.Recommend rejecting empty updates:
if (Object.keys(updateData).length === 0) { throw new Error('No fields provided to update'); }apps/mail/lib/themes/theme-utils.ts (3)
1-3: LGTM!The import statements are correctly structured and necessary for the theme utilities.
499-510: Verify handling of undefinedrootColors.The function passes
item.themeData?.rootColorstogenerateThemeCss, which could beundefined. EnsuregenerateThemeCssproperly handles this case.Looking at the
generateThemeCssfunction, it uses default values when colors are undefined, so this should work correctly. However, consider adding explicit validation for better code clarity.
355-355: ```shell
#!/bin/bashDisplay lines 340 to 370 from theme-utils.ts to review generateThemeCss output mapping
sed -n '340,370p' apps/mail/lib/themes/theme-utils.ts
</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
@snehendu098 please cleanup comments, address coderabbit and resolve conflicts, happy to merge and test and award the bounty |
|
@MrgSub working on it ser, was out for a hackathon |
snehendu098
force-pushed
the
feature/themes-customization
branch
from
babd83e to
dc17035
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/mail/lib/themes/theme-utils.ts (2)
108-161: Consolidate duplicatehslToHeximplementations.The
hslToHexfunction is duplicated inapps/mail/components/theme/theme-customizer.tsx. Consider importing and using this utility function there instead of maintaining duplicate implementations.
286-334: Add dark theme CSS generation support.This function only generates light theme CSS. Given that the application supports dark themes (as evidenced by
extractDarkThemeColorsandhasDarkModefunctions), this function should also support generating dark theme CSS.Consider extending this function to accept both light and dark color schemes and generate appropriate CSS for both variants.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/mail/app/globals.css(12 hunks)apps/mail/components/context/theme-context.tsx(1 hunks)apps/mail/components/mail/mail-iframe.tsx(1 hunks)apps/mail/components/mail/mail.tsx(10 hunks)apps/mail/components/theme/theme-customizer.tsx(1 hunks)apps/mail/components/theme/theme-view.tsx(1 hunks)apps/mail/lib/hotkeys/mail-list-hotkeys.tsx(2 hunks)apps/mail/lib/themes/index.tsx(1 hunks)apps/mail/lib/themes/theme-utils.ts(1 hunks)apps/mail/types/index.ts(1 hunks)apps/server/src/db/migrations/0028_soft_blazing_skull.sql(1 hunks)apps/server/src/db/schema.ts(2 hunks)apps/server/src/lib/schemas.ts(2 hunks)apps/server/src/lib/theme-manager.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/mail/lib/themes/index.tsx
- apps/server/src/db/migrations/0028_soft_blazing_skull.sql
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/mail/components/mail/mail-iframe.tsx
- apps/mail/lib/hotkeys/mail-list-hotkeys.tsx
- apps/mail/app/globals.css
- apps/server/src/db/schema.ts
- apps/mail/components/context/theme-context.tsx
- apps/mail/types/index.ts
- apps/mail/components/theme/theme-view.tsx
- apps/mail/components/mail/mail.tsx
- apps/server/src/lib/schemas.ts
- apps/server/src/lib/theme-manager.ts
🔇 Additional comments (11)
apps/mail/lib/themes/theme-utils.ts (5)
38-105: LGTM! Self-assignment issue has been resolved.The
extractThemeColorsfunction now correctly handles the panel color extraction without the self-assignment issue that was previously identified. The error handling and regex-based color extraction logic are well implemented.
164-204: LGTM! Proper hex to HSL conversion implementation.The function correctly implements the hex to HSL conversion algorithm and returns the appropriate space-separated format for CSS custom properties.
207-274: LGTM! Self-assignment issue has been resolved.The
extractDarkThemeColorsfunction now correctly handles the panel color extraction without the self-assignment issue that was previously identified. The implementation mirrors the light theme extraction function appropriately.
277-283: LGTM! Simple and effective theme detection.The
hasDarkModeandhasLightModefunctions correctly use regex patterns to detect theme support in CSS strings.
337-373: LGTM! Well-structured theme data transformation.The function properly handles the transformation from API responses to client theme data, correctly splitting themes into light and dark variants when both are supported.
apps/mail/components/theme/theme-customizer.tsx (6)
1-88: LGTM! Proper imports and comprehensive theme definitions.The component correctly imports the color conversion utilities from
theme-utils.tsand defines comprehensive default themes for both light and dark modes with proper HSL color values.
91-141: LGTM! Well-organized color groups and utility functions.The color grouping provides good organization for the UI, and the rem value parsing/formatting functions properly handle edge cases with appropriate defaults.
142-294: LGTM! Excellent iframe communication strategy.The component implements a robust multi-method approach for iframe theme application with proper fallbacks, debouncing for performance, and script injection for cross-origin communication. The state management is well-structured.
296-349: LGTM! Error handling has been properly implemented.The
handleSaveThemefunction now includes proper error handling with try-catch blocks and user feedback via toast messages, addressing the previously identified issue.
351-422: LGTM! Well-implemented color input with smart positioning.The
ColorInputcomponent provides excellent UX with intelligent color picker positioning that respects viewport boundaries and supports both visual and text-based color editing.
424-697: LGTM! Comprehensive and accessible UI implementation.The component renders a well-structured interface with proper accessibility, conditional rendering for different theme modes, and effective real-time preview using an iframe. The layout is intuitive and functional.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/mail/lib/themes/theme-utils.ts (2)
108-161: Consolidate duplicatehslToHeximplementations.The
hslToHexfunction is duplicated inapps/mail/components/theme/theme-customizer.tsx. Consider importing and using this utility function there instead of maintaining duplicate implementations.
286-334: Add dark theme CSS generation support.This function only generates light theme CSS. Given that the application supports dark themes (as evidenced by
extractDarkThemeColorsandhasDarkModefunctions), this function should also support generating dark theme CSS.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/mail/lib/themes/theme-utils.ts(1 hunks)
🔇 Additional comments (7)
apps/mail/lib/themes/theme-utils.ts (7)
38-105: Function looks well-implemented.The
extractThemeColorsfunction correctly parses CSS variables from the:rootsection and handles error cases appropriately. The logic for extracting panel colors and other theme properties is sound.
164-204: Hex to HSL conversion is correctly implemented.The mathematical logic for converting hex colors to HSL format is accurate and the function handles edge cases properly.
207-274: Dark theme extraction function is well-implemented.The
extractDarkThemeColorsfunction correctly mirrors the light theme extraction logic for the.darkselector and handles errors appropriately.
277-283: Theme mode detection functions are correct.Simple and effective utility functions for detecting dark and light mode support in CSS.
337-373: Theme data generation function is well-structured.The
generateThemeDatafunction correctly transforms API responses into client-side theme data, properly handling both light and dark variants and using the appropriate extraction functions.
1-35: File structure and default values are well-organized.The imports, type definitions, and default theme values are properly structured and use correct HSL formatting.
320-328: Consider browser compatibility for CSS color functions.The
rgb(from var(--shadow-color) r g b / opacity)syntax uses modern CSS color functions that may not be supported in older browsers. Consider providing fallbacks or using a more widely supported approach.What is the browser support status for CSS "rgb(from var() r g b)" color function syntax?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/server/src/lib/theme-manager.ts (2)
4-6: Remove unnecessary constructor.The empty constructor serves no purpose and should be removed as flagged by static analysis.
-export class ThemeManager { - constructor() {} - +export class ThemeManager {
16-20: Improve type safety for theme data transformation.The type casting
as ThemePalletteandas Themewithout validation could lead to runtime errors if the database returns unexpected data structures.Consider adding validation or using a schema validator:
- themeData: row.themeData as ThemePallette, + themeData: themeDataSchema.parse(row.themeData),Also applies to: 37-41, 53-57
apps/server/src/main.ts (2)
184-186: Remove unnecessary type assertion.The type assertion
as typeof theme.$inferInsertis unnecessary since the payload type already excludesuserId. This assertion could hide potential type errors.- async createTheme(payload: Omit<typeof theme.$inferInsert, 'userId'> & { connectionId: string | null }): Promise<typeof theme.$inferSelect[]> { - return await this.mainDo.createTheme(this.userId, payload as typeof theme.$inferInsert); + async createTheme(payload: Omit<typeof theme.$inferInsert, 'userId'> & { connectionId: string | null }): Promise<typeof theme.$inferSelect[]> { + return await this.mainDo.createTheme(this.userId, payload); }
531-531: Consider removing manual updatedAt setting.The schema definition shows that the
themetable has an$onUpdatetrigger that automatically updates theupdatedAtfield. Manually setting it here might be redundant.#!/bin/bash # Description: Verify if the theme table schema has an $onUpdate trigger for updatedAt # Search for the theme table definition to confirm the $onUpdate trigger ast-grep --pattern 'export const theme = createTable($$$)'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
apps/mail/components/mail/mail-display.tsx(1 hunks)apps/mail/components/mail/mail-iframe.tsx(1 hunks)apps/mail/components/mail/mail.tsx(4 hunks)apps/mail/components/mail/thread-display.tsx(3 hunks)apps/mail/components/theme/theme-customizer.tsx(1 hunks)apps/mail/components/theme/theme-view.tsx(1 hunks)apps/mail/components/ui/nav-main.tsx(2 hunks)apps/mail/lib/hotkeys/mail-list-hotkeys.tsx(2 hunks)apps/mail/providers/client-providers.tsx(3 hunks)apps/mail/types/index.ts(1 hunks)apps/server/src/ctx.ts(1 hunks)apps/server/src/db/migrations/0029_loose_lizard.sql(1 hunks)apps/server/src/db/migrations/0030_fresh_ogun.sql(1 hunks)apps/server/src/db/migrations/meta/0029_snapshot.json(17 hunks)apps/server/src/db/migrations/meta/0030_snapshot.json(4 hunks)apps/server/src/db/migrations/meta/_journal.json(1 hunks)apps/server/src/db/schema.ts(2 hunks)apps/server/src/lib/schemas.ts(2 hunks)apps/server/src/lib/theme-manager.ts(1 hunks)apps/server/src/main.ts(3 hunks)apps/server/src/trpc/index.ts(2 hunks)apps/server/src/trpc/routes/themes.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/server/src/ctx.ts
- apps/server/src/db/migrations/0030_fresh_ogun.sql
🚧 Files skipped from review as they are similar to previous changes (14)
- apps/server/src/trpc/index.ts
- apps/mail/lib/hotkeys/mail-list-hotkeys.tsx
- apps/mail/providers/client-providers.tsx
- apps/mail/components/ui/nav-main.tsx
- apps/mail/components/mail/mail-display.tsx
- apps/mail/components/mail/mail-iframe.tsx
- apps/server/src/db/schema.ts
- apps/mail/components/theme/theme-customizer.tsx
- apps/server/src/trpc/routes/themes.ts
- apps/server/src/lib/schemas.ts
- apps/mail/types/index.ts
- apps/mail/components/mail/thread-display.tsx
- apps/mail/components/mail/mail.tsx
- apps/mail/components/theme/theme-view.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: snehendu098
PR: Mail-0/Zero#1323
File: apps/mail/lib/themes/theme-utils.ts:318-318
Timestamp: 2025-06-24T06:22:58.753Z
Learning: In the Mail-0/Zero theme system (apps/mail/lib/themes/theme-utils.ts), when color themes are being applied, all color values come in HSL format, so there's no need for additional format validation when converting colors with hslToHex().
apps/server/src/lib/theme-manager.ts (1)
Learnt from: snehendu098
PR: Mail-0/Zero#1323
File: apps/mail/lib/themes/theme-utils.ts:318-318
Timestamp: 2025-06-24T06:22:58.753Z
Learning: In the Mail-0/Zero theme system (apps/mail/lib/themes/theme-utils.ts), when color themes are being applied, all color values come in HSL format, so there's no need for additional format validation when converting colors with hslToHex().
🧬 Code Graph Analysis (2)
apps/server/src/main.ts (1)
apps/server/src/db/schema.ts (1)
theme(177-191)
apps/server/src/lib/theme-manager.ts (2)
apps/server/src/lib/schemas.ts (4)
Theme(3-13)ThemePallette(198-198)CreateTheme(199-199)UpdateTheme(200-200)apps/server/src/lib/server-utils.ts (1)
getZeroDB(7-11)
🪛 Biome (1.9.4)
apps/server/src/lib/theme-manager.ts
[error] 5-5: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🔇 Additional comments (9)
apps/server/src/db/migrations/meta/_journal.json (1)
209-228: ```shell
#!/bin/bashVerify missing migrations in the journal file
echo "Searching for missing migration entries in apps/server/src/db/migrations/meta/_journal.json..."
for tag in "0028_worried_molecule_man" "0029_common_network" "0029_thin_triathlon" "0030_blue_grandmaster"; do
echo -n "$tag: "
grep -R "$tag" -n apps/server/src/db/migrations/meta/_journal.json || echo "not found"
done</details> <details> <summary>apps/server/src/db/migrations/0029_loose_lizard.sql (1)</summary> `1-14`: **Well-structured migration for theme functionality.** The migration properly establishes the theme table with: - UUID primary key with auto-generation - Appropriate foreign key constraints with cascade deletes - JSONB storage for flexible theme data - Sensible defaults and nullable fields </details> <details> <summary>apps/server/src/db/migrations/meta/0030_snapshot.json (2)</summary> `783-880`: **Theme table schema is consistent with migration.** The `mail0_theme` table definition in the snapshot correctly matches the migration, with proper: - UUID primary key with auto-generation - Foreign keys to user and connection tables with cascade delete - JSONB theme data storage - Public visibility flag with appropriate default --- `402-637`: **OAuth tables appear unrelated to theme feature.** The addition of `mail0_oauth_access_token`, `mail0_oauth_application`, and `mail0_oauth_consent` tables seems unrelated to the theme customization feature described in the PR objectives. ```shell #!/bin/bash # Description: Check if OAuth-related changes are part of this PR # Expected: Verify if these OAuth tables are intentionally included echo "Searching for OAuth-related code changes in this PR..." rg -i "oauth" --type ts --type tsx --type sqlapps/server/src/lib/theme-manager.ts (2)
119-130: Enhance error handling in copyPublicTheme.The method should verify that the user has permission to copy the theme and ensure the theme actually exists before attempting to copy it.
async copyPublicTheme( userId: string, connectionId: string, publicThemeId: string, ): Promise<Theme> { const db = await getZeroDB(userId); - const publicTheme = (await db.findThemeById(publicThemeId)) as Theme | undefined; + const publicTheme = await this.getThemeById('public', publicThemeId); if (!publicTheme || !publicTheme.isPublic) { throw new Error('Public theme not found'); }Likely an incorrect or invalid review comment.
44-58: ```shell
#!/bin/bashRetrieve the full implementation of findPublicThemes in the Durable Object (applicable lines)
rg "async findPublicThemes" -n -A20 apps/server/src/main.ts
</details> <details> <summary>apps/server/src/db/migrations/meta/0029_snapshot.json (1)</summary> `783-880`: **Theme table structure is consistent across migration snapshots.** The theme table definition matches between migration snapshots, ensuring consistency in the migration chain. </details> <details> <summary>apps/server/src/main.ts (2)</summary> `16-16`: **LGTM!** The `theme` import is correctly added to support the new theme functionality. --- `23-23`: **LGTM!** The `like` operator import is appropriately added for search functionality in public themes. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
@MrgSub please review the code |
|
This PR has merge conflicts and has been open for more than 3 days. It will be automatically closed. Please resolve the conflicts and reopen the PR if you'd like to continue working on it. |
4 participants
Description
Created the ability for users to create new themes and apply their created themes in the application
Type of Change
Please delete options that are not relevant.
Areas Affected
Please check all that apply:
Testing Done
Describe the tests you've done:
Security Considerations
For changes involving data or authentication:
Checklist
Screenshots/Recordings
Screencast_from_2025-06-03_00-47-47.mp4
By submitting this pull request, I confirm that my contribution is made under the terms of the project's license.
Summary by CodeRabbit
New Features
UI Improvements
Bug Fixes
Chores
Database
Documentation
Refactor