Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughRefactors mail UI into separate tray and preview components, adds Instrument Sans font and global CSS updates, introduces DateService with tests, adds HTML sanitization for mail previews, updates dependencies (including dompurify), expands test fixtures, and adjusts SDK/Auth logout and unauthorized handling. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 40: Add the missing TypeScript types by adding `@types/dompurify` to
devDependencies so the compiler recognizes DOMPurify types; update package.json
devDependencies to include "@types/dompurify" (matching the installed
"dompurify" dependency) and run the install step, ensuring imports/usages like
DOMPurify.sanitize in preview/index.tsx resolve without type errors.
In `@src/assets/fonts/InstrumentSans/font.css`:
- Around line 3-10: The `@font-face` for 'Instrument Sans' is missing a
font-weight range causing browsers to extrapolate; update the `@font-face` rule
(for the font-family 'Instrument Sans' referencing
InstrumentSans-variable.woff2) to include a font-weight descriptor specifying
the variable font's supported min and max weight (e.g. the actual range such as
100 900) so weight interpolation and font matching work correctly.
In `@src/components/UserChip/index.tsx`:
- Around line 21-27: Move the role="tooltip" off the trigger container and onto
the actual tooltip element that is rendered/portaled (the element created in the
portal/tooltip renderer inside UserChip); update the trigger container (the div
with ref and onMouseEnter/onMouseLeave and functions handleMouseEnter and
setPosition) to include aria-describedby pointing to the tooltip's id, and
ensure the tooltip element has an id and role="tooltip" so screen readers
associate the described content correctly.
- Line 1: Update the misspelled import and component usage from UserCheap to
UserChip: change the import declaration that currently imports UserCheap from
'@internxt/ui' to import UserChip, and update the JSX usage where <UserCheap ...
/> is rendered to <UserChip ... /> so the symbol matches the exported component
name and resolves the TS2305 error.
In `@src/features/mail/components/mailPreview/header/index.tsx`:
- Around line 28-30: The current users.map rendering uses user.email as the
React key for <UserChip> which can collide when the same email appears multiple
times in the same recipients array; update the key generation in the users.map
call (where UserChip is returned) to produce a stable, unique key per item (for
example combine user.email with the map index or a recipient role/id: e.g.,
`${user.email}-${index}` or a dedicated unique id property) so that UserChip has
a non-duplicating key even when identical emails appear multiple times.
In `@src/features/mail/components/mailPreview/preview/index.tsx`:
- Around line 19-25: The external links rendered in the attachments map
(attachments?.map(...) in the preview component) use rel="noreferrer" only;
update the anchor elements to include rel="noopener noreferrer" so you
explicitly enable noopener behavior for legacy browsers and clarity—locate the
JSX inside the attachments mapping where the <a href={attachment}
target="_blank" rel="noreferrer"> is returned and change the rel value
accordingly.
In `@src/features/mail/components/tray/index.tsx`:
- Around line 9-20: TrayList currently forces a loading state and an empty mails
array; update the TrayListProps type to include optional loading?: boolean and
mails?: Mail[] (use the correct Mail item type used by Tray), change the
TrayList signature to accept these props (e.g., ({ folderName, loading, mails }:
TrayListProps) ), and forward them into the Tray component (Tray
loading={loading ?? true} mails={mails ?? []} emptyState={<TrayEmptyState
folderName={folderName} />}) so callers can control loading and mail data while
preserving current defaults; reference TrayListProps, TrayList, Tray and
TrayEmptyState when making the changes.
In `@src/features/mail/MailView.tsx`:
- Line 3: The MailView component imports and calls getMockedMail (from
test-utils/fixtures) on every render which produces random data and causes UI
instability; remove the production dependency on getMockedMail and replace it
with stable data — either fetch real mail data or create a deterministic
dev/mock object used only in MailView, and stabilize it via useMemo or useState
inside the MailView component (e.g., initialize a single mock once and reuse it)
so the mail content does not change on re-renders; also delete the import of
getMockedMail from the top of the file and ensure any references to
getMockedMail in MailView are replaced with the stable data source.
In `@src/routes/guards/ProtectedRoute.tsx`:
- Line 9: The TODO in the ProtectedRoute component is vague and non-standard;
replace the comment "// !TODO: Check if the user already updated the mail to
send him to the Inbox instead" with a clear actionable item: remove the "!"
prefix, use inclusive language, and either implement or create a tracked issue
(e.g., TODO(PB-XXXX): redirect authenticated users to Inbox if they have
configured mail) describing the exact condition (e.g., "if user.mailConfigured
=== true then navigate to /inbox") so future work is traceable; update the
comment next to the ProtectedRoute component (and any related functions like the
route redirection logic) to state whether the logic will be implemented in this
PR or tracked separately with the issue ID.
In `@src/services/date/date.service.test.ts`:
- Around line 32-44: Tests using DateService.fromNow rely on Date.now() and are
flaky; fix by using Vitest fake timers to freeze time during each test: in the
describe block or each test call vi.useFakeTimers() and
vi.setSystemTime(fixedTimestamp) before invoking DateService.fromNow(...) and
then vi.useRealTimers() (or vi.restoreAllMocks()) after to restore; ensure the
fixedTimestamp is chosen so the expected relative strings ('2 hours ago', 'in 3
hours') are deterministic.
In `@src/services/sdk/auth/index.ts`:
- Around line 7-12: The logout flow in logOut currently calls
authClient.logout(token) and only clears
LocalStorageService.instance.clearCredentials() if that call succeeds, so tokens
remain if logout throws; wrap the logout call in a try/finally inside public
logOut to ensure LocalStorageService.instance.clearCredentials() runs regardless
of authClient.logout(token) success or error (optionally log the error in the
catch or let it propagate after clearing), using SdkManager.instance.getAuth()
and LocalStorageService.instance APIs referenced in the method.
In `@src/services/sdk/index.ts`:
- Around line 34-38: The unauthorizedCallback currently calls
AuthService.instance.logOut() with a void so any rejection is swallowed; update
unauthorizedCallback to handle promise rejections from
AuthService.instance.logOut() (when LocalStorageService.instance is truthy) by
appending a .catch handler that logs the error (use existing logger or
console.error) and ensures local credentials are cleared on failure (call
LocalStorageService.instance.clearCredentials() or the equivalent cleanup
method) so failures don’t leave stale auth state.
In `@src/test-utils/fixtures.ts`:
- Around line 213-216: Fix the inconsistent spam mailbox fixture where
totalEmails and unreadEmails are generated independently: ensure unreadEmails is
constrained to be <= totalEmails by first generating a single value (e.g.,
spamTotal) for totalEmails and then generating unreadEmails with a max of that
value (reference the fields totalEmails and unreadEmails in the spam mailbox
object in src/test-utils/fixtures.ts); update the fixture so unreadEmails uses
the already-generated total value instead of an independent random range.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f863a0d7-0449-44c4-941b-bf041c2d638f
⛔ Files ignored due to path filters (6)
package-lock.jsonis excluded by!**/package-lock.jsonsrc/assets/fonts/InstrumentSans/InstrumentSans-Bold.woff2is excluded by!**/*.woff2src/assets/fonts/InstrumentSans/InstrumentSans-Medium.woff2is excluded by!**/*.woff2src/assets/fonts/InstrumentSans/InstrumentSans-Regular.woff2is excluded by!**/*.woff2src/assets/fonts/InstrumentSans/InstrumentSans-SemiBold.woff2is excluded by!**/*.woff2src/assets/fonts/InstrumentSans/InstrumentSans-variable.woff2is excluded by!**/*.woff2
📒 Files selected for processing (22)
index.htmlpackage.jsonsrc/assets/fonts/InstrumentSans/OFL.txtsrc/assets/fonts/InstrumentSans/font.csssrc/components/UserChip/index.tsxsrc/features/mail/MailView.tsxsrc/features/mail/components/mailPreview/header/index.tsxsrc/features/mail/components/mailPreview/index.tsxsrc/features/mail/components/mailPreview/preview/index.tsxsrc/features/mail/components/tray/header/index.tsxsrc/features/mail/components/tray/index.tsxsrc/features/mail/components/tray/searchInput/index.tsxsrc/features/mail/components/tray/trayEmptyState/index.tsxsrc/i18n/services/i18n.service.tssrc/index.csssrc/routes/guards/ProtectedRoute.tsxsrc/services/date/date.service.test.tssrc/services/date/index.tssrc/services/sdk/auth/index.tssrc/services/sdk/index.tssrc/services/sdk/sdk.service.test.tssrc/test-utils/fixtures.ts
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: 7
♻️ Duplicate comments (1)
src/components/UserChip/index.tsx (1)
21-29:⚠️ Potential issue | 🟡 MinorAccessibility: Remove
role="tooltip"from the trigger element.The
aria-describedbyandtooltipIdadditions are correct, but the trigger container (line 27) still hasrole="tooltip"which should only be on the actual tooltip element (line 36). Havingrole="tooltip"on both elements is semantically incorrect.♿ Proposed fix
<div ref={ref} className="relative inline-block" onMouseEnter={handleMouseEnter} onMouseLeave={() => setPosition(null)} - role="tooltip" aria-describedby={position ? tooltipId : undefined} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/UserChip/index.tsx` around lines 21 - 29, Remove the erroneous role="tooltip" from the trigger div in the UserChip component: locate the JSX block that returns the div using ref, handleMouseEnter, setPosition and aria-describedby={position ? tooltipId : undefined} and delete the role prop so only the actual tooltip element (the element using tooltipId, rendered when position is set) retains role="tooltip"; leave aria-describedby in place and ensure the tooltip element still has role="tooltip" and id={tooltipId}.
🤖 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/mailPreview/header/index.tsx`:
- Around line 55-60: The conditional rendering currently uses "attachmentsLength
&& attachmentsLength > 0" which can evaluate to 0 and render that literal;
update the JSX condition to explicitly check greater-than-zero (i.e., replace
the condition with "attachmentsLength > 0") where attachmentsLength is used in
the mail preview header render so the PaperclipIcon and count only render when
attachmentsLength is positive.
In `@src/services/date/date.service.test.ts`:
- Around line 17-20: Tests assume English locale causing brittle expectations;
in the test suite for DateService (referencing DateService.format and relative
formatting assertions) add a beforeEach that explicitly sets the locale to
English (e.g., call DateService.setLocale('en') or dayjs.locale('en')) and
optionally restore/reset afterEach so the global locale cannot leak between
tests; target the describe block containing the tests that use FIXED_DATE and
relative strings like "2 hours ago" so assertions remain deterministic.
In `@src/services/sdk/auth/auth.service.test.ts`:
- Around line 8-20: The test title claims "the token should be cleared" but only
asserts logout was called; update the test to also assert that
LocalStorageService.instance.clearCredentials() is invoked after
AuthService.instance.logOut(), by adding a spy
(vi.spyOn(LocalStorageService.instance, 'clearCredentials')) before calling
logOut and an expect toHaveBeenCalled() after; alternatively, if you prefer not
to assert clearCredentials here, change the test title to reflect that it only
verifies mockedAuthClient.logout(mockedToken) is called.
- Around line 7-20: The test sets spies on LocalStorageService.instance.getToken
and SdkManager.instance.getAuth but does not restore them, which can leak into
other tests; add cleanup to restore spies after each test (e.g., an afterEach
that calls vi.restoreAllMocks() or explicitly calls mockRestore on the spies) so
that LocalStorageService.instance.getToken and SdkManager.instance.getAuth are
reset; ensure this cleanup runs for the AuthService.instance.logOut test and any
others in the describe block.
In `@src/services/sdk/index.ts`:
- Line 42: The code calls NavigationService.instance.navigate({ id:
AppView.Welcome }) before the async logOut() finishes, allowing navigation while
logout is in progress; either await the logout call (e.g., await
authService.logOut() or ensure the function containing logOut returns a Promise
and await it) before calling NavigationService.instance.navigate, or if
immediate redirect is intentional add an inline comment above
NavigationService.instance.navigate explaining that navigation should be
immediate and logout can continue in background; update the function containing
logOut and NavigationService.instance.navigate to implement the chosen approach
and keep method names logOut and NavigationService.instance.navigate as the
reference points.
- Around line 36-43: The unauthorizedCallback currently calls
LocalStorageService.instance.clearCredentials() before
AuthService.instance.logOut(), which causes AuthService.logOut() (which calls
LocalStorageService.instance.getToken() and ultimately authClient.logout(token))
to receive null; reorder the calls so AuthService.instance.logOut() is invoked
first (so it can read the existing token via
LocalStorageService.instance.getToken() and call authClient.logout(token)), then
call LocalStorageService.instance.clearCredentials() and finally
NavigationService.instance.navigate({ id: AppView.Welcome }).
In `@src/services/sdk/sdk.service.test.ts`:
- Around line 188-196: The test "When Users client receives unauthorized
response, then logOut should be called" currently triggers the
unauthorizedCallback but only asserts AuthService.instance.logOut; update the
test to also assert that NavigationService.instance.navigate was called with the
welcome view (verify NavigationService.instance.navigate was invoked with an
object containing id: AppView.Welcome) after calling
securityArg.unauthorizedCallback(), ensuring NavigationService is properly
spied/mocked in the test setup before invocation.
---
Duplicate comments:
In `@src/components/UserChip/index.tsx`:
- Around line 21-29: Remove the erroneous role="tooltip" from the trigger div in
the UserChip component: locate the JSX block that returns the div using ref,
handleMouseEnter, setPosition and aria-describedby={position ? tooltipId :
undefined} and delete the role prop so only the actual tooltip element (the
element using tooltipId, rendered when position is set) retains role="tooltip";
leave aria-describedby in place and ensure the tooltip element still has
role="tooltip" and id={tooltipId}.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f91a8536-0ed7-45d9-b79b-ac5fbcb5a866
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
package.jsonsrc/components/UserChip/index.tsxsrc/features/mail/components/mailPreview/header/index.tsxsrc/services/date/date.service.test.tssrc/services/sdk/auth/auth.service.test.tssrc/services/sdk/auth/index.tssrc/services/sdk/index.tssrc/services/sdk/sdk.service.test.ts
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/features/mail/MailView.tsx (1)
3-3:⚠️ Potential issue | 🔴 CriticalWire
MailViewto real mailbox state before merging.This composition is still placeholder-driven:
getMockedMail()regenerates a random message on every render, andTrayListcurrently hardcodesloading={true}/mails={[]}insrc/features/mail/components/tray/index.tsx:92-109. As merged, the screen can never show the actual folder contents or selected message, so please replace the fixture/placeholder wiring with real state or at least freeze any temporary mock behind stable state while the data layer lands. Please verify the full folder → selection → preview flow before merging.Also applies to: 14-18, 25-27
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/mail/MailView.tsx` at line 3, MailView currently uses getMockedMail() which regenerates on each render and TrayList is hardcoded with loading={true} and mails={[]} so the UI can't show real folder/selection state; update MailView to stop calling getMockedMail() directly on render and instead wire it to the real mailbox state (or a stable local state) by reading from the mailbox store/prop (e.g., useMailbox, mailboxState, or pass down mails/folders) and ensure TrayList receives dynamic props (loading flag and mails array) from that state rather than hardcoded values; if the backend isn't ready, initialize a stable mocked state once (use useState/useMemo to freeze a fixture) and connect selection handlers so the folder → selection → preview flow (TrayList selection -> selectedMail in MailView -> MailPreview) works end-to-end.src/components/user-chip/index.tsx (1)
22-39:⚠️ Potential issue | 🟠 MajorUse a focusable inline trigger for the tooltip.
The root element is a mouse-only
<div>withrole="tooltip", so keyboard users can't reveal the recipient details, and embedding this component insideRecipientLine's<span>wrappers produces invalid DOM nesting. Render the trigger as an inline focusable element (button/span), moverole="tooltip"to the portaled popup only, and mirror the open/close logic on focus/blur. Please verify the recipient chips with keyboard-only navigation before merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/user-chip/index.tsx` around lines 22 - 39, Replace the root non-focusable div trigger with a focusable inline element (e.g., a <button type="button"> or a <span tabIndex={0}>) so keyboard users can open the tooltip; keep the same className and ref usage (ref) but remove role="tooltip" from the trigger and ensure the portaled popup created by createPortal retains role="tooltip" and id={tooltipId}; mirror mouse handlers by adding onFocus={handleMouseEnter} and onBlur={() => setPosition(null)} alongside onMouseEnter/onMouseLeave so open/close logic (handleMouseEnter, setPosition, tooltipId) works for keyboard and pointer, and keep aria-describedby on the trigger conditional on position (aria-describedby={position ? tooltipId : undefined}) to maintain accessibility.
🤖 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/mail-preview/header/index.tsx`:
- Around line 46-48: Recipient labels are hardcoded; update header to use
localized strings instead of literals by pulling translations via the i18n hook
or by accepting translated props from MailView. Replace the hardcoded "To:",
"CC:", "BCC:" passed to RecipientLine with localized values (e.g., const t =
useTranslation()/useI18n(); then t('mail.to'), t('mail.cc'), t('mail.bcc')) or
change MailView to pass translated strings into this header component (e.g.,
props.toLabel, props.ccLabel, props.bccLabel) and use those in RecipientLine.
Ensure the translation keys are named consistently with the repo's i18n keys and
update call sites (MailView) if you choose the prop approach.
In `@src/features/mail/components/mail-preview/preview/index.tsx`:
- Around line 10-16: The HTML is being sanitized with DOMPurify.sanitize(body)
but remote images/tracking pixels remain allowed and will load when injected via
dangerouslySetInnerHTML; update the sanitization to disallow remote URIs (or
strip image/link tags) before rendering: either call DOMPurify.sanitize(body, {
ALLOWED_URI_REGEXP: /^(?:data:|\/|#)/i }) to only allow data and relative/local
URIs, or pre-process the incoming body (e.g., remove/neutralize <img>, <iframe>,
and <link rel> tags with http(s) src/href) and then assign the result to
sanitizedBody used by the component that calls dangerouslySetInnerHTML; ensure
the change targets the sanitizedBody construction (DOMPurify.sanitize) and the
component rendering that value.
---
Duplicate comments:
In `@src/components/user-chip/index.tsx`:
- Around line 22-39: Replace the root non-focusable div trigger with a focusable
inline element (e.g., a <button type="button"> or a <span tabIndex={0}>) so
keyboard users can open the tooltip; keep the same className and ref usage (ref)
but remove role="tooltip" from the trigger and ensure the portaled popup created
by createPortal retains role="tooltip" and id={tooltipId}; mirror mouse handlers
by adding onFocus={handleMouseEnter} and onBlur={() => setPosition(null)}
alongside onMouseEnter/onMouseLeave so open/close logic (handleMouseEnter,
setPosition, tooltipId) works for keyboard and pointer, and keep
aria-describedby on the trigger conditional on position
(aria-describedby={position ? tooltipId : undefined}) to maintain accessibility.
In `@src/features/mail/MailView.tsx`:
- Line 3: MailView currently uses getMockedMail() which regenerates on each
render and TrayList is hardcoded with loading={true} and mails={[]} so the UI
can't show real folder/selection state; update MailView to stop calling
getMockedMail() directly on render and instead wire it to the real mailbox state
(or a stable local state) by reading from the mailbox store/prop (e.g.,
useMailbox, mailboxState, or pass down mails/folders) and ensure TrayList
receives dynamic props (loading flag and mails array) from that state rather
than hardcoded values; if the backend isn't ready, initialize a stable mocked
state once (use useState/useMemo to freeze a fixture) and connect selection
handlers so the folder → selection → preview flow (TrayList selection ->
selectedMail in MailView -> MailPreview) works end-to-end.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: da29e532-c292-4084-a938-04d33a0be40a
📒 Files selected for processing (5)
src/components/user-chip/index.tsxsrc/features/mail/MailView.tsxsrc/features/mail/components/mail-preview/header/index.tsxsrc/features/mail/components/mail-preview/index.tsxsrc/features/mail/components/mail-preview/preview/index.tsx
larryrider
approved these changes
Mar 20, 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 mail preview component so the mail view is completed (Tray list + preview mail component).
We will need to handle the attachments, but it will be done in another task.