Conversation
WalkthroughThis update introduces label ordering and drag-and-drop reordering functionality for mail labels. It adds a sortable list UI using Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LabelsPage (UI)
participant useLabelOrders Hook
participant TRPC/Backend
participant Database
User->>LabelsPage (UI): Drag label to reorder
LabelsPage (UI)->>useLabelOrders Hook: Call updateOrders(newOrder)
useLabelOrders Hook->>TRPC/Backend: Mutation: reorder(labelOrders)
TRPC/Backend->>Database: Update mail0_label_order with new orders
Database-->>TRPC/Backend: Confirm update
TRPC/Backend-->>useLabelOrders Hook: Success response
useLabelOrders Hook-->>LabelsPage (UI): Refetch orders/labels
LabelsPage (UI)-->>User: Updated label order displayed
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (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: 6
🧹 Nitpick comments (5)
apps/server/package.json (1)
60-60: Pinningnanoidwithout caret may hinder patch-level security updates
"nanoid": "5.1.5"is added as a strict version. This prevents npm from applying non-breaking patch upgrades automatically (e.g. 5.1.x). Unless you have a reproducibility requirement, consider:- "nanoid": "5.1.5", + "nanoid": "^5.1.5",Also double-check whether
uuidis still needed—it overlaps withnanoidfor ID generation.apps/mail/hooks/use-labels.ts (1)
12-14: Relying solely on backend ordering makes the UI fragileThe comment says the server already sorts the labels. If that assumption ever breaks (e.g. cache fallback, older client hitting newer server), the UI will silently render unsorted data. A cheap safeguard is to sort defensively on the client:
- // Labels are already sorted by order from the server - return labelQuery; + // Server should send pre-sorted labels, but sort defensively just in case + return { + ...labelQuery, + data: labelQuery.data?.toSorted((a, b) => (a.order ?? 0) - (b.order ?? 0)), + };This keeps behaviour correct even when the contract drifts.
apps/mail/types/index.ts (1)
10-10: Optionalordershould be nullable for clearer semanticsMost DB rows will have a numeric order, while system labels (Inbox, Sent, …) may intentionally have
NULL. Explicitly declaring this helps TypeScript catch misuse:- order?: number; + order?: number | null;This also matches how Drizzle/SQL returns nullable columns.
apps/server/src/trpc/routes/label.ts (1)
201-212: Consider returning empty object instead of null.Returning
nullwhen no orders exist might require additional null checks on the client side.Consider always returning an object for consistency:
-return Object.keys(ordersMap).length > 0 ? ordersMap : null; +return ordersMap;apps/server/src/routes/chat.ts (1)
744-776: Optimize ID generation in updateLabelOrders.Currently generating a new ID for each upsert operation, even when updating existing records.
Consider checking if the record exists before generating a new ID:
await db.transaction(async (tx) => { for (const { id: labelId, order } of labelOrders) { + // Check if record exists + const existing = await tx + .select({ id: labelOrder.id }) + .from(labelOrder) + .where(and( + eq(labelOrder.connectionId, connectionId), + eq(labelOrder.labelId, labelId) + )) + .limit(1); + await tx .insert(labelOrder) .values({ - id: nanoid(), + id: existing[0]?.id || nanoid(), connectionId, labelId, order, }) .onConflictDoUpdate({ target: [labelOrder.connectionId, labelOrder.labelId], set: { order, updatedAt: new Date(), }, }); } });
📜 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 (15)
apps/mail/app/(routes)/settings/labels/page.tsx(5 hunks)apps/mail/components/labels/label-dialog.tsx(4 hunks)apps/mail/components/ui/sidebar-labels.tsx(3 hunks)apps/mail/hooks/use-label-orders.ts(1 hunks)apps/mail/hooks/use-labels.ts(1 hunks)apps/mail/locales/en.json(1 hunks)apps/mail/types/index.ts(1 hunks)apps/server/package.json(1 hunks)apps/server/src/db/migrations/0035_shocking_green_goblin.sql(1 hunks)apps/server/src/db/migrations/meta/0035_snapshot.json(1 hunks)apps/server/src/db/migrations/meta/_journal.json(1 hunks)apps/server/src/db/schema.ts(1 hunks)apps/server/src/main.ts(1 hunks)apps/server/src/routes/chat.ts(9 hunks)apps/server/src/trpc/routes/label.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.918Z
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.
apps/mail/components/labels/label-dialog.tsx (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().
apps/mail/components/ui/sidebar-labels.tsx (1)
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.918Z
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.
apps/mail/app/(routes)/settings/labels/page.tsx (1)
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.918Z
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.
🔇 Additional comments (12)
apps/mail/hooks/use-label-orders.ts (1)
1-16: LGTM! Clean and well-structured hook implementation.The hook follows React Query and TRPC best practices with appropriate caching strategy and clean API design.
apps/server/src/db/schema.ts (1)
214-234: Well-designed schema for label ordering.The
labelOrdertable has proper foreign key relationships, appropriate defaults, and necessary constraints. The high default order value (999999) is a good pattern for allowing new items to appear at the end.apps/mail/components/ui/sidebar-labels.tsx (4)
50-50: Good addition of folder order tracking.The
folderOrderrecord properly tracks the order of folder groups based on the first label encountered in each group.
71-75: Correct sorting implementation for folder groups.The sorting logic properly orders folder groups by their tracked order values with appropriate fallback to a high number (999999) for consistent behavior.
81-88: Proper label sorting within groups.Labels are correctly sorted by their order property before mapping, ensuring consistent ordering within each folder group.
123-130: Consistent sorting for bracketed labels.The bracketed labels in the "Other" folder are sorted using the same pattern as other label groups, maintaining consistency across the UI.
apps/mail/components/labels/label-dialog.tsx (2)
67-67: Good alignment with server-side color assignment.Changing the default color to empty strings allows the server to assign random colors for new labels, which is consistent with the new label management system.
70-73: Clear documentation of the color assignment strategy.The comment and empty default values properly communicate that the server handles color assignment for new labels.
apps/mail/locales/en.json (1)
347-348: Appropriate localization for label reordering feature.The success and failure messages for label reordering are clear, consistent with existing patterns, and provide good user feedback.
apps/mail/app/(routes)/settings/labels/page.tsx (1)
205-220: Let’s inspect bothupdateLabelOrdersdefinitions inchat.tsand trace themainDoimplementation to see if transactions or locking are used:#!/bin/bash # 1. Show the route handler at line ~744 in chat.ts echo "----- chat.ts (lines 730-800) -----" sed -n '730,800p' apps/server/src/routes/chat.ts # 2. Find where `mainDo` is imported in chat.ts echo "----- mainDo import -----" grep -R "import .*mainDo" -n apps/server/src/routes/chat.ts # 3. Locate the service file implementing updateLabelOrders in mainDo # (replace the path below once we see the import) # 4. Once we have the file, dump its updateLabelOrders body # For now, search for the method across the entire server directory: echo "----- service implementation search -----" rg -n "updateLabelOrders" apps/server/srcapps/server/src/trpc/routes/label.ts (1)
55-62: LGTM! Well-implemented label ordering logic.The implementation correctly merges label orders with labels, applies custom colors, and sorts by order with a sensible default value.
apps/server/src/db/migrations/meta/0035_snapshot.json (1)
1-1195: Auto-generated migration snapshot looks correct.The schema definition for
mail0_label_ordertable matches the SQL migration file, including all constraints and defaults.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Merge activity
|
ahmetskilinc
force-pushed
the
new-labels-system
branch
from
0b975b1 to
91d55c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/mail/app/(routes)/settings/labels/page.tsx (2)
65-67: Handle undefined label.id more safely.Using
label.id || ''could cause issues if the label ID is legitimately undefined.Consider adding validation:
-const { attributes, listeners, setNodeRef, transform, transition, isDragging } = useSortable({ - id: label.id || '', -}); +if (!label.id) { + console.error('Label without ID detected:', label); + return null; +} +const { attributes, listeners, setNodeRef, transform, transition, isDragging } = useSortable({ + id: label.id, +});
121-121: Avoid non-null assertion operator.The non-null assertion
label.id!could cause runtime errors.Since we're already checking for label.id existence above (after applying the previous fix), we can safely use it:
-onClick={() => onDelete(label.id!)} +onClick={() => onDelete(label.id)}
📜 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 (15)
apps/mail/app/(routes)/settings/labels/page.tsx(5 hunks)apps/mail/components/labels/label-dialog.tsx(5 hunks)apps/mail/components/ui/sidebar-labels.tsx(3 hunks)apps/mail/hooks/use-label-orders.ts(1 hunks)apps/mail/hooks/use-labels.ts(1 hunks)apps/mail/messages/en.json(1 hunks)apps/mail/types/index.ts(1 hunks)apps/server/package.json(1 hunks)apps/server/src/db/migrations/0035_shocking_green_goblin.sql(1 hunks)apps/server/src/db/migrations/meta/0035_snapshot.json(1 hunks)apps/server/src/db/migrations/meta/_journal.json(1 hunks)apps/server/src/db/schema.ts(1 hunks)apps/server/src/main.ts(1 hunks)apps/server/src/routes/chat.ts(5 hunks)apps/server/src/trpc/routes/label.ts(4 hunks)
✅ Files skipped from review due to trivial changes (3)
- apps/server/src/main.ts
- apps/server/package.json
- apps/mail/messages/en.json
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/mail/hooks/use-labels.ts
- apps/server/src/db/migrations/meta/_journal.json
- apps/mail/types/index.ts
- apps/mail/hooks/use-label-orders.ts
- apps/mail/components/ui/sidebar-labels.tsx
- apps/server/src/db/schema.ts
- apps/mail/components/labels/label-dialog.tsx
- apps/server/src/db/migrations/0035_shocking_green_goblin.sql
- apps/server/src/routes/chat.ts
- apps/server/src/db/migrations/meta/0035_snapshot.json
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.918Z
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.
apps/mail/app/(routes)/settings/labels/page.tsx (1)
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.918Z
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.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
apps/server/src/trpc/routes/label.ts (1)
198-206: Good validation implementation!The validation for non-negative and unique order values properly addresses potential data integrity issues.
ahmetskilinc
force-pushed
the
new-labels-system
branch
from
7640761 to
021d262
Compare
dnd labels in label settings fixes rebase
ahmetskilinc
force-pushed
the
new-labels-system
branch
from
021d262 to
95f91f1
Compare
2 participants
Add drag-and-drop reordering for labels
This PR implements a drag-and-drop interface for reordering labels in the settings page. Labels can now be visually rearranged using a drag handle, and their order is persisted to the database. The order is respected throughout the application, including in the sidebar navigation.
Technical changes:
@dnd-kitfor drag-and-drop functionality in the labels settings pagemail0_label_orderto store label ordering and custom colorsSummary by CodeRabbit
New Features
Bug Fixes
Chores