Made the email appear on fullscreen #391
Conversation
|
@rohanshrma222 is attempting to deploy a commit to the Inbox Zero Team on Vercel. A member of the Team first needs to authorize it. |
|
|
WalkthroughThis pull request introduces and refactors several email-related components. A new Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant M as Mail Component
participant API as Email API
participant AT as Atom (refetch)
U->>M: Load email page with URL slug
M->>API: Fetch email threads (with pagination)
API-->>M: Return threads data
M->>AT: Update refetch function for global access
alt Viewing specific thread
M->>UI: Render EmailPanel for the selected thread
else
M->>UI: Render EmailList
end
sequenceDiagram
participant U as User
participant MR as MailRedirect Component
participant R as Router
U->>MR: Request the /mail page
MR->>MR: Extract search parameters (type, threadId)
alt threadId exists
MR->>R: Redirect to specific thread view
else
MR->>R: Redirect to general mail list view
end
sequenceDiagram
participant U as User
participant EL as EmailList Component
participant R as Router
participant EP as EmailPanel Component
U->>EL: Click on an email thread
EL->>R: Navigate to URL with type and thread id
R-->>EP: Load updated email panel view
EP->>EP: Check view mode using pathname
alt Fullscreen mode detected
EP->>UI: Render "Back to inbox" button
else
EP->>UI: Render without back button
end
Assessment against linked issues
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
Scope: all 2 workspace projects This error happened while installing a direct dependency of /tmp/eslint/packages/eslint-config ✨ 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:
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: 0
🧹 Nitpick comments (3)
apps/web/components/email-list/EmailPanel.tsx (1)
35-36: Validatepathnamebefore splitting.
Relying onpathname.split("/")might lead to unexpected results ifpathnameis empty or undefined. Consider a defensive check or default value to avoid potential runtime errors.apps/web/app/(app)/mail/[...slug]/page.tsx (2)
34-49: SWR Infinite key generation.
AppendingnextPageTokenfor subsequent pages is good. Consider adding a max page or fallback to avoid infinite scrolling loops.
69-92: In-memory removal of archived threads.
The approach for updating local SWR data helps keep the UI in sync. Consider logging or a user notification if many threads are removed at once.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/app/(app)/mail/[...slug]/page.tsx(1 hunks)apps/web/app/(app)/mail/page.tsx(1 hunks)apps/web/components/email-list/EmailList.tsx(3 hunks)apps/web/components/email-list/EmailPanel.tsx(4 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
apps/web/app/(app)/mail/[...slug]/page.tsx (2)
apps/web/components/email-list/EmailPanel.tsx (1)
EmailPanel(11-107)apps/web/components/email-list/EmailList.tsx (1)
List(37-160)
🔇 Additional comments (21)
apps/web/components/email-list/EmailPanel.tsx (3)
1-1: Imports look appropriate.
No immediate issues with the introduction ofArrowLeftIconandusePathname.Also applies to: 9-9
45-65: Fullscreen logic and “Back to inbox” button implementation.
Good addition of a dedicated back navigation for fullscreen mode. The UI changes are clear and consistent with the new route structure.
78-85: Conditional “Close” button toggle.
Hiding the “Close” button when in fullscreen helps avoid UI clutter. The fallback flow looks correct.apps/web/components/email-list/EmailList.tsx (3)
35-35: New hooks imported successfully.
UsinguseRouteranduseSearchParamsaligns with Next.js best practices for route-based logic.
452-454:❓ Verification inconclusive
Navigating to the new URL structure.
The route push is straightforward. Verify thatthread.idis always defined to prevent potential undefined route segments.
🏁 Script executed:
#!/bin/bash # Checking references to ensure thread.id is defined rg -A 5 "onOpen.*thread.id"Length of output: 29
Action: Ensure Valid
thread.idValueThe current implementation routes with
router.push(/mail/${type}/${thread.id}), which assumes thatthread.idis always defined. Since the initial verification script did not yield output confirming thatthread.idis reliably set, please manually verify that everythreadinstance used in this component guarantees the presence of anid. This could be ensured via type-checking or by having an explicit default/fallback in place.
- Manual Check Needed: Confirm that the source (or parent component) passing
threadenforces a validid.- Guard/Fallback Consideration: If there's any chance that
thread.idmight be undefined, consider adding a conditional check before invokingrouter.push.
182-185:❓ Verification inconclusive
Deriving mail type from search parameters.
Defaulting to "inbox" is a good fallback for a missingtype. Ensure downstream logic handles unexpected search param values gracefully.Would you like to confirm that only known mail types are passed? You can verify it across the codebase to see if additional checks or a fallback might be needed.
🏁 Script executed:
#!/bin/bash rg -A 5 "router.push.*mail/.*"Length of output: 977
Mail Type Handling: Validate Allowed Values
Defaulting to
"inbox"when notypeis provided is a sensible fallback. However, the code currently passes the value from the search parameters directly into routing (e.g.,router.push(\/mail/${type}/${thread.id}`)` in EmailList and similar in the mail page). Please verify that only expected, allowed mail types reach these routes. If there’s a chance that an unexpected value might be present, consider adding explicit validation or mapping to ensure that only known types are used, thereby preventing potential downstream issues.apps/web/app/(app)/mail/page.tsx (3)
3-4: Importing from Next.js.
This makes sense for executing the redirection logic (useEffect,useRouter,useSearchParams).
7-11:MailRedirectcomponent signature and parameter extraction.
The approach of retrievingtypeandthreadIdfrom search params is consistent with the new route logic.
14-20: Conditional redirection.
Usingrouter.replacefor both thread and list views is clear. Confirm any edge cases (e.g., invalidtype) are handled.Do you want to cover additional fallback routes if
typeis invalid or unknown?apps/web/app/(app)/mail/[...slug]/page.tsx (12)
3-6: Imports for SWR, atoms, routing, and UI.
All additions look correct for data fetching and state management.
17-26: ExtractingmailTypeandthreadIdfrom slug.
This is a neat way to handle dynamic routes. Validate usage for any potential empty slug scenario.
28-32: Handling label-based queries vs. mail type.
Great distinction for applyingquery.labelIdorquery.typeautomatically.
51-56: SWR Infinite config.
UsingkeepPreviousDataand a smallerdedupingIntervalhelps performance. The approach is sound.
58-62: DerivingisLoadingMoreandshowLoadMore.
Straightforward logic ensures a smooth user experience.
64-67: Finding the selected thread.
Correctly filters bythreadId. Confirm what happens ifthreadIdis invalid or not found.
94-99: Registering the refetch method in an atom.
Makes sense for globally accessible refetch triggers.
100-102: Load more handler.
Incrementing size is the correct approach for paginated SWR data.
104-107:closePanelwith router push.
Returns user to the mail list for the currentmailType. Matches the full-screen flow.
108-119: Placeholder callback implementations.
Plan for future expansions. Good placeholder approach.
121-147: Rendering EmailPanel when in thread view.
Uses the selected thread logic properly. The skeleton for hooking upexecutePlanand other actions is ready.
149-170: Rendering email list when not in thread view.
Clean fallback. Good usage of<List>fromEmailList.tsx.
| @@ -31,6 +32,8 @@ export function EmailPanel({ | |||
| refetch: () => void; | |||
| }) { | |||
| const isPlanning = useIsInAiQueue(row.id); | |||
| const pathname = usePathname(); | |||
| const isFullscreen = pathname.split("/").length > 3; // Checking if we're in /mail/type/threadId | |||
There was a problem hiding this comment.
super hacky. better to pass in props
There was a problem hiding this comment.
you can't rely on the url having 3+ parts to decide how to show the component
|
Hey, would be best if we did this like Gmail and kept the split screen option as it's already working. But also offered full screen mode. Also I notice the path for the page is /mail/inbox/messageid. may also be worth considering keeping the page as is and the full screen is more a modal type thing. similar to split screen mode, but it just covers the whole screen instead. this way you don't lose page context / need reload when you go back to inbox |
|
Thanks for the suggestion.
|
Why not both ? Modal and new page button ? |
This PR makes the email appear on full screen,Removed the split screen mode.
Fixes #344
Result Video attached :
fix.344.mp4
Summary by CodeRabbit
New Features
UI Enhancements