feat: ability to customize categories#1328
Conversation
WalkthroughThis update introduces dynamic category management to the mail application. It adds a categories settings page, a hook for merging user and default categories, updates schemas to support category settings, and refactors mail components to use dynamic category data. Navigation and localization are updated to include the new categories feature. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CategoriesSettingsPage
participant useCategorySettings
participant Server
User->>CategoriesSettingsPage: Visit /settings/categories
CategoriesSettingsPage->>useCategorySettings: Fetch and merge categories
useCategorySettings->>Server: Get user settings (categories)
useCategorySettings-->>CategoriesSettingsPage: Return merged categories
User->>CategoriesSettingsPage: Edit and Save categories
CategoriesSettingsPage->>Server: Save updated categories
Server-->>CategoriesSettingsPage: Confirm save
CategoriesSettingsPage-->>User: Show success/failure notification
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ 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: 8
🔭 Outside diff range comments (1)
apps/mail/components/mail/mail.tsx (1)
189-208:toast.promiseoptions:finallyis not a recognised key
sonner’stoast.promiseacceptsloading,success, anderror.
The cleanup logic infinallywill be ignored. Call.finallyon the promise instead:-toast.promise(EnableBrain(), { - loading: 'Enabling autolabeling...', - success: 'Autolabeling enabled successfully', - error: 'Failed to enable autolabeling', - finally: async () => { - await refetchBrainState(); - }, -}); +toast.promise( + EnableBrain().finally(refetchBrainState), + { + loading: 'Enabling autolabeling...', + success: 'Autolabeling enabled successfully', + error: 'Failed to enable autolabeling', + }, +);Repeat for the Disable branch below.
🧹 Nitpick comments (7)
apps/mail/config/navigation.ts (1)
175-179: Missingidfield for new nav itemEvery other primary nav item owns an
id(e.g.labels,inbox). Down-stream components occasionally rely on that identifier for selection logic. Add one now to stay consistent.{ - title: 'navigation.settings.categories', + id: 'categories', + title: 'navigation.settings.categories', url: '/settings/categories', icon: Tabs, },apps/mail/app/(routes)/settings/categories/page.tsx (3)
68-77: Inefficient merge – leverage existing hook
useCategorySettingsalready implements merge + sort.
Replacing this manualuseEffectkeeps behaviour in one place and reduces cognitive load.-const [categories, setCategories] = useState<CategorySetting[]>([]); -... -useEffect(() => { ... }); +const { categories, setCategories } = useCategorySettings(); // expose setter in hook
80-83: Typing: avoidany
valueis typed asanywhich defeats type-safety.-const handleFieldChange = (id: string, field: keyof CategorySetting, value: any) => { +const handleFieldChange = <K extends keyof CategorySetting>( + id: string, + field: K, + value: CategorySetting[K], +) => {
110-118: Hard-coded strings bypass i18nTitles/labels should use the locale JSON keys to stay translatable (
SettingsCardtitle, button text, validation toast).apps/mail/hooks/use-categories.ts (1)
80-84: Multiple defaults not handled / re-computing constant data
- If two categories have
isDefault: true,findreturns the first and silently keeps the second – worth asserting or warning.defaultCategoriesis recreated on every render; hoist it outside the hook (file-scope) to avoid needless allocations.apps/mail/components/mail/mail.tsx (2)
825-875: Heavy switch-statement & translation helper need tightening
- The big
switchduplicates a lot of boiler-plate. A static map reduces noise and eases future maintenance.cat.id.toLowerCase().replace(' ', '')only removes the first space; use/\s+/gto handle any whitespace.Example refactor:
const ICON_MAP: Record<string, JSX.Element> = { Important: <Lightning … />, 'All Mail': <Mail … />, Personal: <User … />, Promotions: <Tag … />, Updates: <Bell … />, Unread: <ScanEye … />, }; const categories = categorySettings.map((cat) => ({ ...cat, name: cat.name || t(`common.mailCategories.${cat.id.toLowerCase().replace(/\s+/g, '')}` as any), icon: ICON_MAP[cat.id], }));This also gracefully leaves
iconundefined for unknown categories without casting toany.
909-912: Redundant second default lookup
useDefaultCategoryId()is already executed inMailLayout. Re-computing inside everyCategorySelectmount adds extra work. Consider lifting the hook up and passing thedefaultCategoryIdas a prop to avoid duplicate memoisation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/mail/app/(routes)/settings/categories/page.tsx(1 hunks)apps/mail/app/routes.ts(1 hunks)apps/mail/components/mail/mail.tsx(6 hunks)apps/mail/config/navigation.ts(1 hunks)apps/mail/hooks/use-categories.ts(1 hunks)apps/mail/locales/en.json(1 hunks)apps/server/src/lib/schemas.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/mail/config/navigation.ts (1)
apps/mail/components/icons/icons.tsx (1)
Tabs(594-611)
apps/mail/hooks/use-categories.ts (2)
apps/mail/hooks/use-settings.ts (1)
useSettings(5-17)apps/server/src/lib/utils.ts (1)
c(13-23)
🔇 Additional comments (2)
apps/mail/app/routes.ts (1)
56-57: Route added successfully – verify auth guardThe new
/settings/categoriesroute is wired correctly.
Double-check that the settings layout already protects children behind the same authentication guard as other settings pages so this one isn’t exposed anonymously.apps/mail/locales/en.json (1)
358-359: Nice – translation key present
navigation.settings.categoriesadded and matches the nav config.
…ategories from the server schema
…ept, so no NaN (or other invalid) numbers can make their way into state
…que value for orders
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/mail/hooks/use-categories.ts (2)
6-12:idunion blocks future custom categories
Previous review still applies – constrainingidto six literals prevents user-defined categories that the settings UI implicitly allows.
Change tostring(or a branded string) to stay forward-compatible.
24-38: Custom (non-default) categories are silently dropped
The merge only iterates overdefaultCategories, so a user-added category that isn’t in the defaults disappears at runtime.
See prior suggestion to includeoverrides.filter(o => !defaultCategories.some(d => d.id === o.id)).
🧹 Nitpick comments (3)
apps/server/src/lib/prompts.ts (1)
264-274: Consider deduplicating the category-mapping blockThe Gmail and Outlook prompts now embed identical (but soon to diverge) XML for default categories.
Extract a single TS constant (e.g.,DEFAULT_CATEGORY_MAPPINGS_XML) and interpolate it into each prompt to avoid manual drift when the list is updated or localized.apps/mail/app/(routes)/settings/categories/page.tsx (2)
120-128: Switch can still leave zero defaults in component stateTurning off the only default sets every category’s
isDefaulttofalse, leaving an interim state that fails validation at save time.
While the toast prevents persistence, consider disallowing the toggle-off when it is the sole default, to avoid user confusion.onCheckedChange={(val) => { - const newCats = categories.map((c) => ({ - ...c, - isDefault: c.id === cat.id ? val : false, - })); + if (!val && cat.isDefault && categories.filter(c => c.isDefault).length === 1) { + // Ignore attempt to unset the last default + return; + } + const newCats = categories.map((c) => ({ + ...c, + isDefault: c.id === cat.id ? val : false, + })); setCategories(newCats); }}
229-238:Number()accepts hex & scientific notation – preferparseIntWhen parsing the order value,
Number('0x10')⇒16andNumber('1e3')⇒1000, which are unlikely to be intended.
parseInt(val, 10)avoids these surprises.-const parsed = val === '' ? undefined : Number(val); +const parsed = val === '' ? undefined : parseInt(val, 10);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/mail/app/(routes)/settings/categories/page.tsx(1 hunks)apps/mail/components/mail/mail.tsx(7 hunks)apps/mail/hooks/use-categories.ts(1 hunks)apps/server/src/lib/prompts.ts(2 hunks)apps/server/src/lib/schemas.ts(2 hunks)apps/server/src/trpc/index.ts(1 hunks)apps/server/src/trpc/routes/categories.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/server/src/trpc/index.ts
- apps/server/src/trpc/routes/categories.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/mail/components/mail/mail.tsx
- apps/server/src/lib/schemas.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/mail/app/(routes)/settings/shortcuts/page.tsx (1)
74-80: Untranslated hard-coded “Show ” prefix + potential undefined access
label = \Show ${cat.name}`bakes English into the UI. Use i18n, e.g.label = t('pages.settings.shortcuts.showLabel', { name: cat.name })`.The guard
categorySettings.lengthis fine only if the hook always returns an array.
If it can beundefinedduring loading,categorySettings.lengthwill throw.
Either default the hook to[]or change the check tocategorySettings?.length.apps/mail/lib/hotkeys/mail-list-hotkeys.tsx (1)
222-228: Duplicate mapping logic – extract to a shared utilityThe same hard-coded index mapping now lives here and in
settings/shortcuts/page.tsx. Divergence is likely over time. Export one shared helper (e.g.getCategoryIdForAction(action: 'showImportant' | …)) fromuse-categories.tsand import it in both places.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/mail/app/(routes)/settings/shortcuts/page.tsx(3 hunks)apps/mail/lib/hotkeys/mail-list-hotkeys.tsx(2 hunks)
| const switchCategoryByIndex = useCallback( | ||
| (idx: number) => { | ||
| const cat = categories[idx]; | ||
| if (!cat) return; | ||
| switchMailListCategory(cat.id); | ||
| }, | ||
| [categories, switchMailListCategory], | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Index-based category lookup suffers from the same reordering problem
switchCategoryByIndex(idx) relies on categories[idx]. Once categories are reordered by the user, hot-keys will target the wrong category.
Reuse a stable id mapping instead:
const actionId: Record<number, string> = {
...
};
const cat = categories.find(c => c.id === actionId[idx]);or expose a helper that resolves showImportant → category.id directly to keep the logic in one place and avoid duplication with the shortcuts page.
🤖 Prompt for AI Agents
In apps/mail/lib/hotkeys/mail-list-hotkeys.tsx around lines 202 to 209, the
function switchCategoryByIndex uses the index to access categories, which breaks
when categories are reordered. To fix this, replace the index-based lookup with
a stable mapping from index to category ID, then find the category by ID in the
categories array. Alternatively, create or reuse a helper function that maps
shortcut actions like 'showImportant' directly to category IDs to centralize and
avoid duplicating this logic.
There was a problem hiding this comment.
looked into this, creating a hardcoded stable mapping does not make sense, since user will update it
also, hot-keys will target the wrong category. does not take place.
There was a problem hiding this comment.
🧩 Analysis chain
I need to better understand how the category system works to verify this behavior. Let me examine the implementation details.
🏁 Script executed:
#!/bin/bash
# Let's examine the category schemas and default categories
echo "=== Category Schema and Default Categories ==="
cat apps/server/src/lib/schemas.ts | grep -A 30 "defaultMailCategories"
echo -e "\n=== Category Settings Hook ==="
cat apps/mail/hooks/use-categories.ts
echo -e "\n=== How Categories are used in the mail component ==="
cat apps/mail/components/mail/mail.tsx | grep -A 20 -B 5 "Categories"
Length of output: 6146
@retrogtx Thank you for the clarification! You're right to dispute my analysis. Looking at the code, I can see the theoretical issue where reordering categories by changing their order property would shift array indices and break the hardcoded hotkey mappings. However, you have the actual implementation context that I'm missing.
Could you help me understand how the category reordering actually works in practice? For instance:
- Are there UI constraints that prevent users from reordering the specific default categories that the hotkeys target?
- Does the reordering mechanism maintain some stable relationship between hotkeys and their intended categories?
- Or is the reordering feature limited to certain types of categories while keeping the default ones fixed?
I'd like to understand the real-world behavior so I can provide more accurate feedback in the future.
✏️ Learnings added
Learnt from: retrogtx
PR: Mail-0/Zero#1328
File: apps/mail/lib/hotkeys/mail-list-hotkeys.tsx:202-209
Timestamp: 2025-06-18T17:26:50.908Z
Learning: In apps/mail/lib/hotkeys/mail-list-hotkeys.tsx, the switchCategoryByIndex function using hardcoded indices for category hotkeys does not break when users reorder categories, contrary to the theoretical index-shifting issue. The actual implementation has constraints or mechanisms that prevent hotkey targeting issues.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
2 participants
Description
Users can now customize the mail categories (
All,Important,Personal,Promotions) by modifying:Things remaining to make it better:
Blocking issue:
The button should be at the top right, since the user needs to scroll way at the bottom. However rest of the app so far has the save button at the bottom of the screen so this follows the same thing. Should change for this page upon discussion ofc.
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
Add screenshots or recordings here if applicable.
updated version:
https://x.com/amritwt/status/1935183583033049268
By submitting this pull request, I confirm that my contribution is made under the terms of the project's license.
Summary by CodeRabbit
Summary by CodeRabbit