Conversation
📝 WalkthroughWalkthroughAdds a Preferences UI: modal dialog, section components and list, navigation hook using URL search params, i18n keys, account popover and settings integration in the mail header, preference types and constants, layout mounting, and removal of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/preferences/components/Section.tsx`:
- Around line 22-35: The icon-only buttons in the Section component (the back
button wired to onBackButtonClicked and the close button that calls onClose)
lack accessible names and the back button lacks an explicit type; update both
buttons to include descriptive aria-label attributes (e.g., aria-label="Back"
and aria-label="Close") and add type="button" to the back button (and optionally
to the close button) so they won’t submit parent forms and will be announced by
screen readers.
- Line 16: The Section functional component declaration currently annotates its
return type as "JSX.Element"; remove the explicit ": JSX.Element" from the arrow
function signature in the Section component so TypeScript can infer the return
type from the JSX, e.g., edit the Section(...) => JSX.Element line to
Section(...) => { ... } (remove only the return-type annotation) while keeping
props and implementation unchanged.
In `@src/components/preferences/components/SectionItem.tsx`:
- Line 21: The clickableContainerClass currently includes two conflicting hover
utilities; update the conditional in SectionItem.tsx so clickableContainerClass
only contains a single hover:bg-* class (remove either hover:bg-gray-1 or
hover:bg-gray-5) based on the intended design, e.g., change const
clickableContainerClass = isClickable ? 'hover:bg-gray-5' : ''; to use only the
chosen utility; ensure the rest of the component uses clickableContainerClass as
before.
- Around line 20-40: The component currently only alters styling for isDisabled
but still attaches onClick and renders a clickable Element; update SectionItem
to prevent interaction when isDisabled is true by conditionally omitting the
onClick prop and by setting the button-specific disabled attribute when Element
=== 'button' (e.g., pass disabled={isDisabled}), and add accessibility
attributes like aria-disabled={isDisabled} and tabIndex={isDisabled ? -1 : 0} so
disabled items cannot be activated or focused; ensure you still apply the visual
disabled classes (isDisabled, disabledTextClass) but do not call the onClick
handler when isDisabled is true.
In `@src/components/preferences/index.tsx`:
- Around line 20-24: The effect using useEffect that updates document.title when
isOpen is true (checking isOpen and using title) currently never restores the
previous title; modify the effect to capture the current document.title before
setting it (e.g., const prev = document.title) and return a cleanup that
restores document.title = prev when the dialog closes (or when isOpen becomes
false) so the original tab title is reinstated; ensure this logic lives inside
the same useEffect that depends on [isOpen, title].
In `@src/components/preferences/sections/general/index.tsx`:
- Line 8: Replace the JSX comment {/* TODO: Add appearance, language and support
components */} so static analysis no longer flags an open TODO — either convert
it to a tracked-task reference (e.g. replace TODO with TODO(`#ISSUE_ID`) or TODO:
GH-1234) that points to a real issue, or remove the "TODO" keyword and reword it
to a non-TODO note (e.g. "Pending: add appearance, language and support
components — tracked in ISSUE_ID"); update the comment in index.tsx accordingly
so Sonar no longer reports the unresolved TODO.
In `@src/features/mail/components/settings/components/account-popover/index.tsx`:
- Around line 45-47: The text currently renders
translate('accountPopover.spaceUsed', { space: percentageUsed }) with
percentageUsed optional; guard this before rendering to avoid showing
"undefined% space used". Update the account-popover component to only call/
render the <p> that uses translate when percentageUsed is not null/undefined (or
provide a safe default like 0) by checking the percentageUsed variable before
invoking translate; locate the usage of percentageUsed and translate in the
account-popover/index.tsx component and wrap or short-circuit the render so
translate never receives undefined.
- Around line 75-85: The Item component renders a clickable <div> which is not
keyboard-accessible; change Item to render a semantic <button> (e.g., in the
Item function) instead of the <div>, remove role="none", add type="button", keep
the existing className, style and onClick handler so appearance and behavior are
preserved, and ensure any necessary ARIA attributes (aria-label) are added where
the logout Item is used to maintain accessibility.
In `@src/features/mail/components/settings/index.tsx`:
- Around line 30-32: The settings Button currently renders only a GearIcon which
is not accessible to screen readers; update the Button component (the Button
with props variant="ghost" and onClick={openPreferences}) to include an
accessible name such as aria-label="Open settings" or aria-label="Open mail
preferences" so assistive technologies can announce the control while keeping
the GearIcon as its visual content.
- Line 16: The calculation for percentageUsed can produce Infinity when limit is
0; update the guard around the expression that defines percentageUsed so it only
computes Math.round((usage / limit) * 100) if usage is not null/undefined and
limit is a finite number greater than 0 (e.g., check usage != null &&
Number.isFinite(limit) && limit > 0), otherwise return 0; modify the variable
named percentageUsed accordingly to prevent invalid values from reaching the UI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8dd7e6c4-7dad-4f2e-8d89-730a19447f77
📒 Files selected for processing (14)
src/components/preferences/components/Section.tsxsrc/components/preferences/components/SectionItem.tsxsrc/components/preferences/components/SectionList.tsxsrc/components/preferences/components/SectionListWrapper.tsxsrc/components/preferences/index.tsxsrc/components/preferences/sections/general/index.tsxsrc/context/dialog-manager/types/index.tssrc/features/mail/MailView.tsxsrc/features/mail/components/settings/components/account-popover/index.tsxsrc/features/mail/components/settings/index.tsxsrc/hooks/preferences/usePreferencesNavigation.tsxsrc/i18n/locales/en.jsonsrc/routes/layouts/SidebarAndHeaderLayout.tsxsrc/types/preferences/index.ts
💤 Files with no reviewable changes (1)
- src/context/dialog-manager/types/index.ts
src/features/mail/components/settings/components/account-popover/index.tsx
Show resolved
Hide resolved
src/features/mail/components/settings/components/account-popover/index.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/features/mail/components/settings/components/account-popover/index.tsx (1)
49-55:⚠️ Potential issue | 🟡 MinorAdd
type="button"to prevent implicit form submission.Both the settings button (line 49) and the
Itembutton (line 77) lacktype="button". Without it, buttons default totype="submit", which can cause unexpected form submissions if these are ever nested in a form.🛡️ Proposed fix
<button + type="button" className="flex w-full cursor-pointer items-center px-3 py-2 text-gray-80 no-underline hover:bg-gray-1 hover:text-gray-80 dark:hover:bg-gray-10" onClick={openPreferences} >function Item({ children, onClick }: Readonly<ItemProps>) { return ( <button + type="button" className="flex cursor-pointer items-center px-3 py-2 text-gray-80 hover:bg-gray-1 dark:hover:bg-gray-10"Also applies to: 77-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/mail/components/settings/components/account-popover/index.tsx` around lines 49 - 55, The two button elements in account-popover (the settings button that calls openPreferences and the Item button rendering at/near the Item component) are missing an explicit type attribute and will default to type="submit" inside a form; update both button elements to include type="button" to prevent implicit form submission, i.e., add type="button" to the button that invokes openPreferences and to the button rendered by the Item entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/preferences/components/SectionItem.tsx`:
- Around line 1-9: The SectionItemProps interface and its consumer component
expose unused props (isSection, isSubsection, isDisabled, notificationsNumber)
while callers in SectionList only use text, isActive, and onClick; tighten the
API by removing those dead props from SectionItemProps and from the SectionItem
component signature and implementation (remove any handling/JSX related to
isSection/isSubsection/isDisabled/notificationsNumber), and ensure SectionList
continues to render SectionItem with only text, isActive, and onClick —
alternatively, if you prefer keeping the props, wire SectionList to pass them
through from its source, but do one or the other to avoid a misleading contract.
In `@src/features/mail/components/settings/components/account-popover/index.tsx`:
- Around line 65-67: The inline arrow `() => panel` passed to the Popover's
panel prop creates a new function reference each render; refactor by extracting
that function into a stable handler (e.g., const renderPanel = () => panel)
declared outside the JSX or, if Popover accepts an element, pass the panel
element directly (panel) instead of a function; update the JSX to use
renderPanel (or panel) and keep references like Popover, panel and avatarWrapper
intact so the change prevents unnecessary re-renders.
- Line 21: The component signature uses the removed global JSX namespace causing
CI to fail; update the return type in the AccountPopover component to a
supported React type — e.g., change "JSX.Element" to "React.JSX.Element" or to
"React.ReactNode" (or import React and use "React.JSX.Element") in the function
that accepts Readonly<AccountPopoverProps>; ensure any necessary React
import/typing is present so the type resolves (search for the AccountPopover
function in index.tsx to apply the fix).
---
Duplicate comments:
In `@src/features/mail/components/settings/components/account-popover/index.tsx`:
- Around line 49-55: The two button elements in account-popover (the settings
button that calls openPreferences and the Item button rendering at/near the Item
component) are missing an explicit type attribute and will default to
type="submit" inside a form; update both button elements to include
type="button" to prevent implicit form submission, i.e., add type="button" to
the button that invokes openPreferences and to the button rendered by the Item
entry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a0d39740-c697-4825-be08-c2624fcd8352
📒 Files selected for processing (4)
src/components/preferences/components/SectionItem.tsxsrc/components/preferences/index.tsxsrc/features/mail/components/settings/components/account-popover/index.tsxsrc/features/mail/components/settings/index.tsx
src/features/mail/components/settings/components/account-popover/index.tsx
Outdated
Show resolved
Hide resolved
src/features/mail/components/settings/components/account-popover/index.tsx
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/components/preferences/components/Section.tsx (1)
18-31:⚠️ Potential issue | 🟠 MajorAdd accessible names and explicit button types for icon-only buttons.
Line 18 and Line 26 render icon-only controls without accessible names, and Line 18 also omits
type="button". This hurts screen-reader usability and can accidentally submit a parent form.♿ Suggested fix
- <button onClick={onBackButtonClicked}> + <button type="button" aria-label="Back" onClick={onBackButtonClicked}> <div className="mr-2.5 flex h-9 w-9 items-center justify-center rounded-lg text-gray-100 hover:bg-highlight/4 active:bg-highlight/8"> <CaretLeftIcon size={22} /> </div> </button> @@ <button + type="button" + aria-label="Close preferences" className="flex h-9 w-9 items-center justify-center rounded-md hover:bg-highlight/4 active:bg-highlight/8" - onClick={() => onClose()} + onClick={onClose} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/preferences/components/Section.tsx` around lines 18 - 31, The two icon-only buttons in the Section component (the back button using onBackButtonClicked / CaretLeftIcon and the close button using onClose / XIcon) need accessible names and explicit button types: add type="button" to the back button (and ideally to the close button) to avoid accidental form submit, and add an accessible name via aria-label or aria-labelledby (e.g., aria-label="Back" for the CaretLeftIcon button and aria-label="Close" for the XIcon button) so screen readers announce their purpose.src/features/mail/components/settings/components/account-popover/index.tsx (1)
75-84: 🧹 Nitpick | 🔵 TrivialAdd
type="button"andw-fullfor consistency with the settings button.The Item component's button should explicitly set
type="button"to prevent form submission behavior if reused in a form context. Additionally, addingw-fullensures the logout item matches the full-width styling of the settings button above it.♻️ Proposed fix
function Item({ children, onClick }: Readonly<ItemProps>) { return ( <button - className="flex cursor-pointer items-center px-3 py-2 text-gray-80 hover:bg-gray-1 dark:hover:bg-gray-10" + type="button" + className="flex w-full cursor-pointer items-center px-3 py-2 text-gray-80 hover:bg-gray-1 dark:hover:bg-gray-10" style={{ lineHeight: 1.25 }} onClick={onClick} > {children} </button> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/mail/components/settings/components/account-popover/index.tsx` around lines 75 - 84, The Item component's button (function Item) should explicitly set type="button" to avoid accidental form submissions and include the w-full utility to match the full-width styling of the settings button; update the JSX in Item to add type="button" and append "w-full" to the className string so the button is full width and non-submitting when used inside forms.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/mail/components/settings/components/account-popover/index.tsx`:
- Line 29: The separator constant uses an invalid Tailwind class
"border-translate"; update the separator JSX (constant separator) to use the
correct top-border class (e.g., replace "border-translate" with "border-t") so
the divider renders as a horizontal line while keeping the existing spacing and
color classes ("mx-3 my-0.5 border-gray-10"); ensure you update the className
string in the separator declaration inside the account-popover component.
In `@src/features/mail/components/settings/index.tsx`:
- Around line 24-26: The onLogout handler currently just dispatches
logoutThunk() which can reject if AuthService.instance.logOut() fails, leaving
local state intact and the user without feedback; update onLogout to await or
attach a .catch to dispatch(logoutThunk()) so you can handle rejection: on error
show user feedback (toast/dialog) and still clear local state by dispatching
userActions.resetState() or invoking the thunk’s local-logout path (or prefer
updating logoutThunk to always clear state and navigate via
NavigationService.instance.navigate({ id: AppView.Welcome })); reference
onLogout, logoutThunk, AuthService.instance.logOut, userActions.resetState, and
NavigationService when making the change.
---
Duplicate comments:
In `@src/components/preferences/components/Section.tsx`:
- Around line 18-31: The two icon-only buttons in the Section component (the
back button using onBackButtonClicked / CaretLeftIcon and the close button using
onClose / XIcon) need accessible names and explicit button types: add
type="button" to the back button (and ideally to the close button) to avoid
accidental form submit, and add an accessible name via aria-label or
aria-labelledby (e.g., aria-label="Back" for the CaretLeftIcon button and
aria-label="Close" for the XIcon button) so screen readers announce their
purpose.
In `@src/features/mail/components/settings/components/account-popover/index.tsx`:
- Around line 75-84: The Item component's button (function Item) should
explicitly set type="button" to avoid accidental form submissions and include
the w-full utility to match the full-width styling of the settings button;
update the JSX in Item to add type="button" and append "w-full" to the className
string so the button is full width and non-submitting when used inside forms.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7a761366-33a9-4314-8f54-28e897e1c8dd
📒 Files selected for processing (4)
src/components/preferences/components/Section.tsxsrc/components/preferences/components/SectionItem.tsxsrc/features/mail/components/settings/components/account-popover/index.tsxsrc/features/mail/components/settings/index.tsx
|
|
||
| const avatarWrapper = <Avatar diameter={36} style={{ minWidth: 36 }} fullName={fullName} src={user.avatar} />; | ||
|
|
||
| const separator = <div className="border-translate mx-3 my-0.5 border-gray-10" />; |
There was a problem hiding this comment.
Typo: border-translate is not a valid Tailwind class.
This appears to be a typo. If you intend to render a horizontal divider with a top border, use border-t instead.
🐛 Proposed fix
- const separator = <div className="border-translate mx-3 my-0.5 border-gray-10" />;
+ const separator = <div className="border-t mx-3 my-0.5 border-gray-10" />;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const separator = <div className="border-translate mx-3 my-0.5 border-gray-10" />; | |
| const separator = <div className="border-t mx-3 my-0.5 border-gray-10" />; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/mail/components/settings/components/account-popover/index.tsx`
at line 29, The separator constant uses an invalid Tailwind class
"border-translate"; update the separator JSX (constant separator) to use the
correct top-border class (e.g., replace "border-translate" with "border-t") so
the divider renders as a horizontal line while keeping the existing spacing and
color classes ("mx-3 my-0.5 border-gray-10"); ensure you update the className
string in the separator declaration inside the account-popover component.
Comment on lines
+24
to
+26
| const onLogout = () => { | ||
| dispatch(logoutThunk()); | ||
| }; |
There was a problem hiding this comment.
**Consider handling logout errors gracefully.**Looking at the logoutThunk from the context snippet, if the API call to AuthService.instance.logOut() fails, the thunk rejects without clearing local state. The user remains logged in with no feedback. "Error handling and feedback: Provide clear error messages or feedback to users if there are any issues when they sign out."
The current implementation silently fails if the network request errors. Consider either:
- Handling the error in the thunk to always clear local state (graceful degradation)
- Or catching the rejected promise here and showing user feedback
🔧 Option 1: Clear local state even if API fails (in logoutThunk)
// In src/store/slices/user/thunks/logOutThunk/index.ts
export const logoutThunk = createAsyncThunk<void, void, { state: RootState }>(
'user/logout',
async (_: void, { dispatch }) => {
try {
await AuthService.instance.logOut();
} catch {
// Log error but continue with local logout
console.error('Server logout failed, clearing local session');
}
dispatch(userActions.resetState());
NavigationService.instance.navigate({ id: AppView.Welcome });
},
);🔧 Option 2: Handle rejection in the component
const onLogout = () => {
- dispatch(logoutThunk());
+ dispatch(logoutThunk())
+ .unwrap()
+ .catch(() => {
+ // Show toast or error message
+ });
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/mail/components/settings/index.tsx` around lines 24 - 26, The
onLogout handler currently just dispatches logoutThunk() which can reject if
AuthService.instance.logOut() fails, leaving local state intact and the user
without feedback; update onLogout to await or attach a .catch to
dispatch(logoutThunk()) so you can handle rejection: on error show user feedback
(toast/dialog) and still clear local state by dispatching
userActions.resetState() or invoking the thunk’s local-logout path (or prefer
updating logoutThunk to always clear state and navigate via
NavigationService.instance.navigate({ id: AppView.Welcome })); reference
onLogout, logoutThunk, AuthService.instance.logOut, userActions.resetState, and
NavigationService when making the change.
larryrider
approved these changes
Mar 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adding the Preferences dialog. The popover account has also been added to open the dialog or log out.
This PR only contains the basic setup for the dialog (the component itself and the logic to open/close it). The General view will be added in other PR.