Feat: Added open in new tab option for threads#1552
Feat: Added open in new tab option for threads#1552samrathreddy wants to merge 4 commits intoMail-0:stagingfrom
Conversation
WalkthroughA new popup view for email threads was introduced, including a dedicated route and React components to render threads in a popup window. Context menus and dropdowns now offer an "Open in new tab" action, which opens the thread in a custom-sized popup. Localization strings for related UI messages were also added. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ThreadContextMenu
participant ThreadDisplay
participant Browser
participant PopupThreadDisplay
User->>ThreadContextMenu: Right-click thread, select "Open in new tab"
ThreadContextMenu->>Browser: window.open(popup URL)
alt Popup allowed
Browser->>PopupThreadDisplay: Render thread in popup window
else Popup blocked
ThreadContextMenu->>ThreadContextMenu: Show "Popup blocked" toast
end
User->>ThreadDisplay: Click dropdown, select "Open in new tab"
ThreadDisplay->>Browser: window.open(popup URL)
alt Popup allowed
Browser->>PopupThreadDisplay: Render thread in popup window
else Popup blocked
ThreadDisplay->>ThreadDisplay: Show "Popup blocked" toast
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ 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 (
|
samrathreddy
changed the title
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/mail/components/mail/thread-display.tsx (1)
737-761: Consider extracting shared popup logic to reduce code duplication.The function works correctly but duplicates the same logic found in
thread-context.tsx(lines 270-296). This violates the DRY principle and makes maintenance harder.Consider creating a shared utility function:
// utils/popup-utils.ts export const openThreadInNewTab = (threadId: string, t: (key: string, options?: any) => string) => { try { const baseUrl = window.location.origin; const encodedThreadId = encodeURIComponent(threadId); const popupUrl = `${baseUrl}/popup/thread?threadId=${encodedThreadId}`; const popup = window.open( popupUrl, `thread-${encodedThreadId}`, 'width=800,height=600,resizable=yes,status=no,location=no,toolbar=no,menubar=no' ); if (!popup) { toast.error( t('common.actions.popupBlocked', { fallback: 'Popup blocked. Please allow popups for this site.', }) ); } } catch (error) { console.error('Failed to open popup:', error); toast.error( t('common.actions.failedToOpenPopup', { fallback: 'Failed to open thread in new tab.', }) ); } };Then use it in both components:
const handleOpenInNewTab = useCallback(() => { if (!id) return; openThreadInNewTab(id, t); }, [id, t]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/mail/app/(full-width)/popup/thread/page.tsx(1 hunks)apps/mail/app/routes.ts(1 hunks)apps/mail/components/context/thread-context.tsx(3 hunks)apps/mail/components/mail/mail-display.tsx(1 hunks)apps/mail/components/mail/popup-thread-display.tsx(1 hunks)apps/mail/components/mail/thread-display.tsx(3 hunks)apps/mail/locales/en.json(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: danteissaias
PR: Mail-0/Zero#902
File: apps/mail/components/connection/add.tsx:77-77
Timestamp: 2025-05-07T16:55:46.513Z
Learning: For the "Upgrade" link in AddConnectionDialog, using a proper <button> element instead of a <span> with onClick is recognized as an accessibility improvement but was deferred as out of scope in PR #902 (CSS variables PR).
apps/mail/app/routes.ts (1)
Learnt from: retrogtx
PR: Mail-0/Zero#1468
File: apps/server/src/trpc/routes/mail.ts:386-391
Timestamp: 2025-06-27T04:59:29.709Z
Learning: In apps/server/src/trpc/routes/mail.ts, the attachment processing logic conditionally handles mixed attachment types - it preserves existing File-like objects with arrayBuffer methods while only converting serialized attachments that need processing through toAttachmentFiles.
apps/mail/components/mail/thread-display.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/components/context/thread-context.tsx (2)
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.
Learnt from: danteissaias
PR: Mail-0/Zero#902
File: apps/mail/components/connection/add.tsx:77-77
Timestamp: 2025-05-07T16:55:46.513Z
Learning: For the "Upgrade" link in AddConnectionDialog, using a proper <button> element instead of a <span> with onClick is recognized as an accessibility improvement but was deferred as out of scope in PR #902 (CSS variables PR).
🧬 Code Graph Analysis (2)
apps/mail/app/(full-width)/popup/thread/page.tsx (1)
apps/mail/components/mail/popup-thread-display.tsx (1)
PopupThreadDisplay(10-77)
apps/mail/components/mail/thread-display.tsx (1)
apps/mail/components/icons/icons.tsx (1)
Printer(1748-1763)
🪛 Biome (1.9.4)
apps/mail/components/mail/popup-thread-display.tsx
[error] 18-18: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (12)
apps/mail/components/mail/mail-display.tsx (1)
297-297: LGTM! Clean prop addition for popup context.The optional
isPopupprop is well-designed and maintains backward compatibility while allowing the component to adjust its behavior when rendered in a popup window.apps/mail/app/routes.ts (1)
16-16: LGTM! Route properly configured.The new
/popup/threadroute is correctly placed under the full-width layout, which is appropriate for a popup thread view that needs maximum screen real estate.apps/mail/locales/en.json (2)
60-61: LGTM! Well-crafted user feedback messages.The popup-related error messages are clear and informative, providing users with actionable feedback when popup functionality fails.
214-214: LGTM! Clear UI label for the new feature.The "Open in new tab" label is descriptive and follows the existing localization patterns in the threadDisplay section.
apps/mail/app/(full-width)/popup/thread/page.tsx (2)
18-21: LGTM! Document title setting is appropriate.Setting the document title helps users identify the popup window when multiple tabs/windows are open.
23-29: LGTM! Clean full-screen container structure.The container structure provides appropriate full-screen layout for the popup thread view.
apps/mail/components/context/thread-context.tsx (3)
15-15: Good addition of ExternalLink icon import.The import is properly placed and will be used for the new "Open in new tab" action.
270-296: Well-implemented popup opening logic.The function includes:
- Proper URL encoding for security
- Appropriate popup window features
- Good error handling with user feedback
- Popup blocking detection
The implementation follows best practices for opening popup windows.
320-326: Good integration of the new action.The new "Open in new tab" action is properly integrated into the primaryActions array with appropriate icon, label, and handler.
apps/mail/components/mail/popup-thread-display.tsx (1)
24-76: Well-structured component with proper state handling.The component properly:
- Handles loading and empty states
- Uses appropriate UI components and styling
- Passes correct props to MailDisplay
- Includes proper accessibility and responsive design
The conditional rendering logic is clear and the scrollable area implementation is appropriate.
apps/mail/components/mail/thread-display.tsx (2)
49-49: Good addition of ExternalLink import.The import is properly added and will be used in the dropdown menu.
996-1014: Well-structured dropdown menu additions.The new dropdown menu items are properly implemented:
- Good use of event.stopPropagation() to prevent unintended behavior
- Proper icon and text placement
- Consistent styling with existing items
- Logical placement of the print option alongside the new tab option
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
|
Please move to context menu |
|
@MrgSub Could you please elaborate |
|
|
2 participants
Description
Added option to provide threads in new window
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
Additional Notes
Add any other context about the pull request here.
Screenshots/Recordings
Before:
After:
Screen.Recording.2025-06-30.at.12.31.50.AM.mov
By submitting this pull request, I confirm that my contribution is made under the terms of the project's license.
Summary by CodeRabbit