[FE - package] Account Deletion - Android & WebApp #299
Conversation
mathieubouhelier
requested review from
anicioalexandre and
priscilladeroode
as code owners
|
WalkthroughThe changes implement visual representation for deleted users across multiple components by introducing a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✨ Finishing touches
🧪 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/index.tsx (1)
24-24: Simplify null/undefined check.The check
== null || === undefinedis redundant. The loose equality operator== nullalready matches bothnullandundefined.Apply this diff:
- const isProfileNullOrUndefined = message?.profile == null || message?.profile === undefined + const isProfileNullOrUndefined = message?.profile == null
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/components/modules/activity-log/web/ActivityLogComponent/LogGroups/index.tsx(4 hunks)packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/index.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/components/modules/activity-log/web/ActivityLogComponent/LogGroups/index.tsx (1)
packages/components/modules/activity-log/common/types.ts (1)
LogGroup(7-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Component Test Packages
- GitHub Check: Build and Lint Packages
🔇 Additional comments (5)
packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/index.tsx (3)
52-58: Verify theisRead === nullcondition for top message detection.The logic assumes
previousMessage?.isRead === nullindicates the first message at the top. Ensure this assumption holds across all message states and thatisReadbeingnullis exclusively used as a boundary marker and won't occur in other contexts (e.g., unread messages whereisReadmight befalseinstead).
66-67: Avatar visibility logic refined for deleted users.The updated logic now requires
isFirstGroupedMessageto be true for deleted users to show their avatar, which is more consistent with message grouping behavior. This prevents avatars from appearing on every message from a deleted user within the same group.
95-99: Proper handling of deleted user names.The fallback to "Deleted User" when profile is null/undefined correctly addresses the account deletion requirement.
packages/components/modules/activity-log/web/ActivityLogComponent/LogGroups/index.tsx (2)
3-3: Clean import addition for deleted user avatar handling.The
AvatarWithPlaceholderimport supports the new deleted user functionality.
40-43: Good deleted user handling with helper functions.The
renderUserNameandrenderAvatarhelpers cleanly centralize deleted user logic, showing "Deleted User" text and placeholder avatar whenuseris null. This aligns with the PR objectives for account deletion.Also applies to: 67-70
packages/components/modules/activity-log/web/ActivityLogComponent/LogGroups/index.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/components/modules/activity-log/web/ActivityLogComponent/LogGroups/index.tsx (1)
44-54: The issues from the previous review are still unresolved.The
renderAvatarfunction still contains the same critical issues that were identified in the previous review:
- Line 49:
sizes="small"is not a valid MUI Avatar prop (it's for responsive imagesrcSet, not sizing)- Line 48: Using
styleinstead ofsxis inconsistent with MUI best practices- The
Avatarlacks explicit dimensions, creating visual inconsistency with theAvatarDeleteUserIconApply the previously suggested fix:
const renderAvatar = (group: LogGroup) => { - if (group.logs[0]?.user == null) return <AvatarDeleteUserIcon /> + if (group.logs[0]?.user == null) return <AvatarDeleteUserIcon sx={{ fontSize: 44 }} /> return ( <Avatar - style={{ marginBottom: '4px' }} - sizes="small" + sx={{ marginBottom: '4px', width: 44, height: 44 }} src={group.logs[0]?.user?.avatar?.url ?? ''} alt={group.logs[0]?.user?.fullName ?? 'User activity log avatar'} /> ) }Note: Also added explicit
fontSize: 44toAvatarDeleteUserIconfor size consistency.
🧹 Nitpick comments (1)
packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/index.tsx (1)
25-25: Remove redundant undefined check.The check
message?.profile == nullalready covers bothnullandundefined(using loose equality), making the additional|| message?.profile === undefinedcheck redundant.Apply this diff:
- const isProfileNullOrUndefined = message?.profile == null || message?.profile === undefined + const isProfileNullOrUndefined = message?.profile == null
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/components/modules/activity-log/web/ActivityLogComponent/LogGroups/index.tsx(4 hunks)packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/index.tsx(4 hunks)packages/design-system/components/web/icons/AvatarDeleteUserIcon/index.tsx(1 hunks)packages/design-system/components/web/icons/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/components/modules/activity-log/web/ActivityLogComponent/LogGroups/index.tsx (1)
packages/components/modules/activity-log/common/types.ts (1)
LogGroup(7-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Component Test Packages
- GitHub Check: Build and Lint Packages
🔇 Additional comments (3)
packages/design-system/components/web/icons/index.ts (1)
6-6: LGTM!The export follows the established pattern and is correctly positioned alphabetically.
packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/index.tsx (2)
77-87: Good sizing consistency with AvatarWithPlaceholder.The conditional avatar rendering correctly handles the deleted user case, and the
fontSize: 32override onAvatarDeleteUserIconensures it matches the 32×32 dimensions ofAvatarWithPlaceholder.
53-53: The review comment is incorrect. The=== nullcheck is intentional and correct.The evidence shows
isReadis nullable (Boolean | null):
- GraphQL schema allows
nullreturn values- Mock data explicitly demonstrates three states:
true(read),false(unread), andnull(system-generated messages)- In the codebase, system messages set
isRead: null(e.g., storybook mockResolvers line 111)The strict null check (
=== null) correctly distinguishes this scenario: the check identifies when there's no previous message with a read status to determine grouping. Using== nullor a falsy check would incorrectly conflatefalse(unread user messages) withnull(system messages), breaking the grouping logic.Likely an incorrect or invalid review comment.
packages/design-system/components/web/icons/AvatarDeleteUserIcon/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/design-system/components/web/avatars/ClickableAvatar/index.tsx (1)
22-28: Update accessibility text to reflect deleted user context.The
titleAccessprop still reads "Avatar Fallback" but should be more specific now that we're using a dedicated deleted-user icon.Apply this diff to improve accessibility:
- <AvatarDeleteUserIcon sx={{ fontSize: width }} titleAccess="Avatar Fallback" /> + <AvatarDeleteUserIcon sx={{ fontSize: width }} titleAccess="Deleted User Avatar" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/components/modules/comments/web/CommentItem/CommentReplyButton/index.tsx(1 hunks)packages/components/modules/comments/web/CommentItem/CommentReplyButton/types.ts(1 hunks)packages/components/modules/comments/web/CommentItem/index.tsx(2 hunks)packages/design-system/components/web/avatars/ClickableAvatar/index.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and Lint Packages
- GitHub Check: Component Test Packages
🔇 Additional comments (4)
packages/design-system/components/web/avatars/ClickableAvatar/index.tsx (1)
8-8: LGTM! Semantic improvement.Replacing the generic
AvatarUploadFallbackIconwith the specificAvatarDeleteUserIconimproves semantic clarity for deleted user avatars.packages/components/modules/comments/web/CommentItem/CommentReplyButton/types.ts (1)
6-6: LGTM!The optional
isDisabledproperty is correctly typed and maintains backward compatibility.packages/components/modules/comments/web/CommentItem/index.tsx (1)
143-143: Good refactoring for reusability.Extracting
totalCommentsCountinto a local variable improves readability and avoids repeated property access.packages/components/modules/comments/web/CommentItem/CommentReplyButton/index.tsx (1)
15-15: LGTM!The
isDisabledprop is correctly implemented with a sensible default value and properly forwarded to theIconButton.Also applies to: 19-19
packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/index.tsx
Outdated
Show resolved
Hide resolved
packages/design-system/components/web/icons/AvatarDeleteUserIcon/index.tsx
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/index.tsx (3)
24-24: Simplify null/undefined check.The loose equality check
== nullalready matches bothnullandundefined, so the explicit=== undefinedcheck is redundant.Apply this diff to simplify:
- const isProfileNullOrUndefined = message?.profile == null || message?.profile === undefined + const isProfileNullOrUndefined = message?.profile == null
66-67: Simplify avatar visibility logic.The boolean expression can be factored to reduce duplication of
isFirstGroupedMessage.Apply this diff:
- const canShowAvatar = - (isProfileNullOrUndefined && isFirstGroupedMessage) || (isFirstGroupedMessage && !isOwnMessage) + const canShowAvatar = isFirstGroupedMessage && (isProfileNullOrUndefined || !isOwnMessage)
98-98: Internationalize "Deleted User" string.The hard-coded "Deleted User" string will not adapt to different locales. Consider using your i18n framework to support multiple languages.
For example, if using
react-intlor similar:{isProfileNullOrUndefined ? t('messages.deletedUser') : message?.profile?.name}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/components/modules/activity-log/web/ActivityLogComponent/LogGroups/index.tsx(4 hunks)packages/components/modules/comments/web/CommentItem/index.tsx(2 hunks)packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/index.tsx(3 hunks)packages/design-system/components/web/avatars/AvatarWithPlaceholder/index.tsx(2 hunks)packages/design-system/components/web/avatars/AvatarWithPlaceholder/types.ts(1 hunks)packages/design-system/components/web/avatars/ClickableAvatar/index.tsx(2 hunks)packages/design-system/components/web/icons/AvatarDeletedUserIcon/index.tsx(1 hunks)packages/design-system/components/web/icons/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-02-11T13:55:04.375Z
Learnt from: pt-tsl
Repo: silverlogic/baseapp-frontend PR: 212
File: packages/components/modules/messages/MessagesList/index.tsx:80-80
Timestamp: 2025-02-11T13:55:04.375Z
Learning: Avoid using non-null assertions (!.) in TypeScript code unless there's absolute certainty about non-nullability. Instead, prefer handling null types inside the relevant functions/hooks and use optional chaining (?.) for safer access.
Applied to files:
packages/components/modules/comments/web/CommentItem/index.tsx
🧬 Code graph analysis (1)
packages/components/modules/activity-log/web/ActivityLogComponent/LogGroups/index.tsx (1)
packages/components/modules/activity-log/common/types.ts (1)
LogGroup(7-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and Lint Packages
- GitHub Check: Component Test Packages
🔇 Additional comments (1)
packages/components/modules/messages/web/MessagesList/MessagesGroup/UserMessage/index.tsx (1)
52-52: Verify the logic for determining first message at the top.Using
previousMessage?.isRead === nullas the condition to identify the first message at the top seems unusual. This assumes thatisRead: nullhas a special semantic meaning (e.g., messages above the read marker). Please confirm this logic is correct and aligns with your message-read tracking model.If this is indeed the intended behavior, consider adding a comment explaining why
isRead === nullindicates the first message at the top, as it's not immediately obvious:// isRead is null for messages above the read marker, indicating the top of the unread section const isFirstMessageAtTheTop = previousMessage?.isRead === null
packages/design-system/components/web/icons/AvatarDeletedUserIcon/index.tsx
Show resolved
Hide resolved
anicioalexandre
left a comment
anicioalexandre
left a comment
There was a problem hiding this comment.
friendly reminder to version this PR before merging!
mathieubouhelier
deleted the
feature/delete-account/BA-2686-FE-package-Account-Deletion-Android-WebApp
branch
Acceptance Criteria
As a User, on the web app,I would like to have the ability to delete my account directly from the app, In order to maintain control over my personal data.
Description
Add an option for users to delete their account
Place the "Delete Account" option under the Settings or Profile
Include a confirmation step before deletion is executed to prevent accidental removal
Display a clear message explaining that deletion is permanent and what data will be affected
We normally don't permanently delete accounts, we rather inactivate them however to comply with Apple's guidelines this story needs to enable the permanent account deletion when Users opt for it
Upon confirmation, trigger backend process to delete or anonymize user data
Update frontend to reflect account removal (e.g., navigate to Sign In screen after deletion)
Ensure deletion logs are properly captured for audit purposes
Acceptance Criteria
Given the user is logged in and navigates to the Settings/Profile, when they select “Delete Account,” then they should see a confirmation screen explaining the consequences
Given the user confirms deletion, when the deletion process completes, then the user account and related data are permanently deleted or anonymized according to policy
Given the deletion is completed, when the user tries to access the app again, then they should be redirected to the Sign In screen and be unable to log in with the deleted credentials
Given the feature is reviewed by App Store, when account deletion is checked, then the option is visible and functional within the app
Validation Rules
User must confirm deletion before it is processed
Deleted account credentials must not allow login
Deleted user ID must be removed from active user lists in backend
Business Rules
Comply with App Store policy that requires in-app account deletion if account creation is supported
Notes
Removed from the story
Ensure data deletion meets privacy regulations (e.g., GDPR or CCPA)
Summary by CodeRabbit
New Features
Design System Updates