Conversation
rf2tsl
requested review from
anicioalexandre and
priscilladeroode
as code owners
|
📝 WalkthroughWalkthroughCentralizes group-form constants/types into common, adds zod validation and helpers, extends GroupChatProvider state with Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (UI)
participant Edit as EditGroupChatDetails
participant Picker as ImagePicker
participant Context as GroupChatProvider (useGroupChatCreate)
participant Relay as Relay (Query/Mutation)
User->>Edit: open edit screen (roomId)
Edit->>Relay: fetch GroupDetailsQuery(roomId)
Relay-->>Edit: return room (title, image, participantIds)
Edit->>Context: setExistingParticipants(participantIds)
Edit->>Context: setRoomId(roomId)
User->>Edit: choose "Change Avatar"
Edit->>Picker: open image picker
Picker-->>Edit: selected image/file
Edit->>Context: update groups.image/title
User->>Edit: save
Edit->>Relay: send UpdateChatRoom mutation (image/title/participants)
Relay-->>Edit: return updated room (participantsCount, node...)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/components/modules/messages/web/GroupChatEdit/AddMembersDialog/index.tsx (1)
199-208:⚠️ Potential issue | 🟡 MinorAdd
keyprop to<AddMemberCard>in therenderItemcallback.The
renderItemcallback returns<AddMemberCard>without akeyprop. WhenProfilesListrenders items in non-paginated mode (line 87 ofProfilesList/index.tsx:profiles.map((member) => renderItem(member, true))), the missing key triggers React warnings and can degrade list reconciliation. Useprofile?.idas the key:Show fix
renderItem: (profile) => { if (!profile) return null return ( <AddMemberCard key={profile?.id} profile={profile} handleAddMember={handleAddMember} handleRemoveMember={handleRemoveMember} isBeingAdded={participants.some((member) => member?.id === profile?.id)} isExistingMember={existingMembers.some((member) => member?.id === profile?.id)} /> ) },packages/components/modules/messages/native/GroupDetailsPage/index.tsx (1)
96-104:⚠️ Potential issue | 🔴 Critical
roomIdprop is not forwarded to<Members>.As noted in the
Membersreview, theroomIdprop was added toMembersPropsbut is not passed here, breaking the "Add Member" navigation.Proposed fix
<Members participantsCount={group?.participantsCount} members={members} loadNext={handleLoadMoreMembers} isLoadingNext={isLoadingNext} hasNext={hasNext} + roomId={roomId} />
🤖 Fix all issues with AI agents
In `@packages/components/modules/messages/common/constants.ts`:
- Line 29: The title length limit is inconsistent: the zod schema in
packages/components/modules/messages/common/constants.ts currently uses .max(20,
...) while EditGroupChatDetails/index.tsx sets <TextInput maxLength={30}>; make
them match by updating the zod rule to .max(30, { message: "Title can't be more
than 30 characters" }) in the title schema (the constant that defines title
validation) so the schema and the TextInput maxLength align; alternatively, if
you prefer a 20-char limit, change the TextInput's maxLength to 20 and update
its UI text accordingly—ensure both the validation rule and the TextInput use
the same numeric limit and that the validation message reflects that limit.
- Around line 38-46: Remove the duplicate definitions of getDefaultFormValues
and DEFAULT_FORM_VALIDATION from the web constants by deleting them from the web
copy and update the GroupChatEdit import to use the shared versions: change the
import in GroupChatEdit/index.tsx to import getDefaultFormValues and
DEFAULT_FORM_VALIDATION from the common constants module
(../../common/constants) and remove the identical getDefaultFormValues and
DEFAULT_FORM_VALIDATION declarations from
packages/components/modules/messages/web/GroupChatEdit/constants.ts so there is
a single source of truth.
In
`@packages/components/modules/messages/common/context/GroupChatProvider/index.tsx`:
- Around line 20-21: resetGroupChat currently calls set({
...INITIAL_GROUP_CHAT_STATE }) but INITIAL_GROUP_CHAT_STATE lacks roomId and
Zustand's set does a shallow merge, so a stale roomId can remain; update
INITIAL_GROUP_CHAT_STATE to explicitly include roomId: undefined (in the
GroupChatProvider constants) so spreading it clears roomId, or alternatively
change resetGroupChat to call set(INITIAL_GROUP_CHAT_STATE, true) using the
replace flag; target symbols: resetGroupChat, INITIAL_GROUP_CHAT_STATE,
setRoomId, and the Zustand set call in GroupChatProvider/index.tsx.
In `@packages/components/modules/messages/native/EditGroupChatDetails/index.tsx`:
- Around line 45-48: Remove the leftover console.log in the useCallback for
handleSheetChanges and replace it with the intended handling (or a clear
no-op/TODO) so no debug prints remain; locate the handleSheetChanges function
and either implement the sheet-change logic that updates state or calls the
appropriate prop/callback, or replace the body with a commented TODO/empty
return, and ensure the useCallback dependencies reflect any variables you
reference.
- Line 101: The input value currently uses a truthy conditional on groups.title
which treats an empty string as falsy and falls back to existingTitle; update
the expression in EditGroupChatDetails so it uses an explicit null/undefined
check (or nullish coalescing) to choose groups.title only when it is not
null/undefined, otherwise fall back to existingTitle and then an empty string —
i.e., preserve an empty string value instead of reverting to existingTitle.
- Around line 50-62: The onResult handler is spreading the entire zustand store
object (groups) into setGroupChat, which leaks action functions into state;
update the handler in useViewPhotoLibrary's onResult so it only passes the state
fields needed (e.g., call groups.setGroupChat({ image: image.uri }) or construct
an object with just id, participants, title, roomId, image) instead of {
...groups, image: ... }, and remove the debug console.log('index', index) near
where useViewPhotoLibrary is called; reference: the useViewPhotoLibrary onResult
callback and the groups.setGroupChat invocation.
In
`@packages/components/modules/messages/native/GroupDetailsPage/Members/index.tsx`:
- Around line 38-41: Members is using roomId in the FabButton navigation but
GroupDetailsPage does not pass roomId, causing
router.push(`/edit-group-details/${roomId}/add-members`) to navigate to
"undefined"; update the contract so MembersProps requires roomId (remove the
optional ? from roomId in the Members component) and then pass the actual roomId
from the parent GroupDetailsPage when rendering <Members>, or alternatively add
a guard in Members (check for roomId before calling router.push) and
disable/hide the FabButton when roomId is missing; reference the Members
component, MembersProps, roomId, GroupDetailsPage, FabButton and the router.push
call when making the change.
🧹 Nitpick comments (10)
packages/components/modules/messages/native/EditGroupChatDetails/styles.ts (2)
7-14: Nit:flex: 1already impliesflexGrow: 1in React Native.Both properties are set on
container, butflex: 1already setsflexGrow: 1. This is redundant. That said, the same pattern exists inChatCreate/styles.ts, so this is consistent with the codebase.
40-46:position: 'relative'is the default in React Native and can be removed.In React Native, all views default to
position: 'relative', making this declaration a no-op.packages/components/modules/messages/common/utils.ts (1)
82-91: Consider a dedicatedconstants.tsthat doesn't import from modules causing the cycle.The "not ideal" comments are honest — domain constants in
utils.tsmuddies the module's purpose. If the originalconstants.tscaused a circular dependency, it's likely because it imported from a file that transitively imported back. A leaner constants file with zero internal imports would break the cycle cleanly.Not urgent, but worth tracking as tech debt.
packages/components/modules/messages/common/types.ts (1)
11-15:any[]on all participant arrays loses type safety.
addParticipants,participants, andremoveParticipantsare all typed asany[]. This lets invalid data slip through silently. Consider typing these to the actual participant shape (e.g.,string[]for IDs or a sharedProfileNode/ participant type).packages/components/modules/messages/web/GroupChatEdit/constants.ts (1)
19-22: Pre-existing:as anycast masks a potential schema/default-values mismatch.The default values (Line 12-17) include
addParticipants/removeParticipants, but the validation schema uses aparticipantskey. Theas anycast hides this discrepancy. Not introduced by this PR, but worth addressing when the TODO is tackled.packages/components/modules/messages/web/GroupChatEdit/AddMembersDialog/index.tsx (1)
14-20: ProfileNode import sourced from__shared__/typeswhile sibling components useprofiles/common.
ProfileNodeis defined in bothpackages/components/modules/messages/web/__shared__/types.tsandpackages/components/modules/profiles/common/types.ts. Within theGroupChatEditmodule, sibling components likeAddMemberCard/types.ts(Line 1) and parentindex.tsx(Line 18) importProfileNodefromprofiles/common, butAddMembersDialog/index.tsx(Line 20) sources it from__shared__/types. For consistency with the broader migration pattern and sibling components in the same module, migrate this import toprofiles/common.packages/components/modules/messages/native/GroupDetailsPage/index.tsx (1)
33-37: Stale-closure risk:groupsis excluded from the effect dependency array.
useGroupChatCreate()returns the full store state (viauseStore(store, state => state)), so the returned object identity changes on every state update. Capturing it outside the dep array means the effect closes over a potentially stale reference.Destructure only
setRoomId(which is a stable zustand action) to satisfy exhaustive-deps cleanly:Proposed fix
- const groups = useGroupChatCreate() - - useEffect(() => { - groups.setRoomId(roomId) - }, [roomId]) + const { setRoomId } = useGroupChatCreate() + + useEffect(() => { + setRoomId(roomId) + }, [roomId, setRoomId])packages/components/modules/messages/common/constants.ts (1)
48-51:as anycast suppresses type safety — consider a narrower workaround.This is also duplicated in
web/GroupChatEdit/constants.ts(Lines 18–21). If the zodResolver typing issue is systemic, consider adding a shared utility type or a single well-documented cast, rather than spreadingas anyacross multiple files.The
TODO: fix typing issue with zodResolverappears in multiple places. Would you like me to open an issue to track this?packages/components/modules/messages/native/EditGroupChatDetails/index.tsx (2)
39-39: Stale closure:groups.resetGroupChatis captured once with[]deps.Same pattern as
GroupDetailsPage. Destructure onlyresetGroupChat:Proposed fix
- const groups = useGroupChatCreate() - - useEffect(() => () => groups.resetGroupChat(), []) + const groups = useGroupChatCreate() + const { resetGroupChat } = groups + + useEffect(() => () => resetGroupChat(), [resetGroupChat])
75-76: The three-way image state (undefined/''/ uri) is fragile and undocumented.
groups.image === undefinedmeans "unchanged",''means "removed", and a uri string means "new image". This implicit protocol drives the conditional on Line 75 and thehandleRemoveImagebranching. Consider adding a brief comment or introducing a named helper (e.g.,hasVisibleImage) to make the intent clear to future readers.Also applies to: 85-88
| export const getDefaultFormValues = ( | ||
| title: string, | ||
| image: string | undefined, | ||
| ): CreateOrEditGroup => ({ | ||
| title, | ||
| addParticipants: [], | ||
| removeParticipants: [], | ||
| image, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for all getDefaultFormValues definitions and usages
rg -n 'getDefaultFormValues' --type=ts -A 2 -B 2Repository: silverlogic/baseapp-frontend
Length of output: 2105
🏁 Script executed:
#!/bin/bash
# Read both constants files
echo "=== WEB VERSION ==="
cat packages/components/modules/messages/web/GroupChatEdit/constants.ts
echo -e "\n=== COMMON VERSION ==="
cat packages/components/modules/messages/common/constants.ts
echo -e "\n=== CHECK IMPORTS FROM COMMON VERSION ==="
rg -n 'from.*common/constants' packages/components/modules/messages/ --type=tsRepository: silverlogic/baseapp-frontend
Length of output: 3533
🏁 Script executed:
#!/bin/bash
# Check all imports of getDefaultFormValues
rg -n 'import.*getDefaultFormValues' --type=tsRepository: silverlogic/baseapp-frontend
Length of output: 215
Remove duplicate getDefaultFormValues and DEFAULT_FORM_VALIDATION from web constants.
Both functions are defined identically in packages/components/modules/messages/web/GroupChatEdit/constants.ts and packages/components/modules/messages/common/constants.ts. Update the import in packages/components/modules/messages/web/GroupChatEdit/index.tsx (line 35) to source these from ../../common/constants instead of ./constants, then remove both functions from the web version to maintain a single source of truth.
🤖 Prompt for AI Agents
In `@packages/components/modules/messages/common/constants.ts` around lines 38 -
46, Remove the duplicate definitions of getDefaultFormValues and
DEFAULT_FORM_VALIDATION from the web constants by deleting them from the web
copy and update the GroupChatEdit import to use the shared versions: change the
import in GroupChatEdit/index.tsx to import getDefaultFormValues and
DEFAULT_FORM_VALIDATION from the common constants module
(../../common/constants) and remove the identical getDefaultFormValues and
DEFAULT_FORM_VALIDATION declarations from
packages/components/modules/messages/web/GroupChatEdit/constants.ts so there is
a single source of truth.
| setRoomId: (roomId: string) => set({ roomId }), | ||
| resetGroupChat: () => set({ ...INITIAL_GROUP_CHAT_STATE }), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "constants" --type f --path "*/messages/common/context/GroupChatProvider*"Repository: silverlogic/baseapp-frontend
Length of output: 305
🏁 Script executed:
find packages/components/modules/messages/common/context/GroupChatProvider -name "*constants*" -type fRepository: silverlogic/baseapp-frontend
Length of output: 154
🏁 Script executed:
cat packages/components/modules/messages/common/context/GroupChatProvider/constants.tsRepository: silverlogic/baseapp-frontend
Length of output: 255
🏁 Script executed:
find packages/components/modules/messages/common/context/GroupChatProvider -name "types.ts" -type f -exec cat {} \;Repository: silverlogic/baseapp-frontend
Length of output: 747
🏁 Script executed:
cat -n packages/components/modules/messages/common/context/GroupChatProvider/index.tsxRepository: silverlogic/baseapp-frontend
Length of output: 1670
resetGroupChat may not clear roomId due to Zustand's shallow merge.
INITIAL_GROUP_CHAT_STATE does not include roomId, and Zustand's set performs shallow merge by default. This means calling resetGroupChat will leave a stale roomId from a previous group context.
Ensure INITIAL_GROUP_CHAT_STATE explicitly includes roomId: undefined:
Fix: Add roomId to INITIAL_GROUP_CHAT_STATE (preferred)
In packages/components/modules/messages/common/context/GroupChatProvider/constants.ts:
export const INITIAL_GROUP_CHAT_STATE: GroupChatCreateState = {
id: undefined,
participants: [],
image: undefined,
title: '',
+ roomId: undefined,
}Alternatively, use the replace parameter in resetGroupChat to fully reset state, but adding roomId: undefined to the initial state is simpler and clearer.
🤖 Prompt for AI Agents
In
`@packages/components/modules/messages/common/context/GroupChatProvider/index.tsx`
around lines 20 - 21, resetGroupChat currently calls set({
...INITIAL_GROUP_CHAT_STATE }) but INITIAL_GROUP_CHAT_STATE lacks roomId and
Zustand's set does a shallow merge, so a stale roomId can remain; update
INITIAL_GROUP_CHAT_STATE to explicitly include roomId: undefined (in the
GroupChatProvider constants) so spreading it clears roomId, or alternatively
change resetGroupChat to call set(INITIAL_GROUP_CHAT_STATE, true) using the
replace flag; target symbols: resetGroupChat, INITIAL_GROUP_CHAT_STATE,
setRoomId, and the Zustand set call in GroupChatProvider/index.tsx.
There was a problem hiding this comment.
@rf2tsl what setRoomId does here? since we dont have roomId in the defaults. Isn't this method the same as setGroupChat?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
setRoomId allows me to reset the chatroom details when a person changes screens but are still in the same room, if I reset the room id each time sometimes it errors out probably due to some synchronization issue.
packages/components/modules/messages/native/EditGroupChatDetails/index.tsx
Show resolved
Hide resolved
packages/components/modules/messages/native/EditGroupChatDetails/index.tsx
Show resolved
Hide resolved
packages/components/modules/messages/native/EditGroupChatDetails/index.tsx
Show resolved
Hide resolved
packages/components/modules/messages/native/GroupDetailsPage/Members/index.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@packages/components/modules/messages/common/context/GroupChatProvider/types.ts`:
- Line 6: The type for the existingParticipants property uses the wrapper type
String instead of the primitive string; change existingParticipants?: String[]
to use the primitive (existingParticipants?: string[]) and also update any other
occurrences using String (notably the other instance at the same file) so all
array/string types use lowercase string to stay consistent with the rest of the
interface declarations (look for existingParticipants and the other String[]
occurrences in types.ts).
🧹 Nitpick comments (2)
packages/components/modules/messages/native/ChatCreate/CreateRoomList/CreateGroupListItem/index.tsx (1)
32-37: Consider computingisSelectedas a value rather than a function call.
isSelectedis a pure derivation of render-time state — defining it as a function that's immediately invoked adds no benefit. A simpleconst(oruseMemoif the lists grow large) would be more idiomatic and slightly clearer.Suggested simplification
- const isSelected = () => { - const { existingParticipants } = groups - const currentParticipants = groups.participants?.map((participant) => participant.id) || [] - const allParticipants = [...(existingParticipants || []), ...currentParticipants] - return allParticipants.some((participant) => participant === node.id) - } + const currentParticipantIds = groups.participants?.map((p) => p.id) || [] + const isSelected = + groups.existingParticipants?.includes(node.id) || currentParticipantIds.includes(node.id)Then on line 54:
- status={isSelected() ? 'checked' : 'unchecked'} + status={isSelected ? 'checked' : 'unchecked'}packages/components/modules/messages/native/GroupDetailsPage/index.tsx (1)
44-49: Missinggroups(or its setters) in theuseEffectdependency array.
groups.setExistingParticipantsandgroups.setRoomIdare invoked inside the effect but not listed in deps. If these setters are stable references (e.g., from Zustand), this is safe in practice but will trigger anexhaustive-depslint warning. Consider destructuring the setters outside and adding them to the dep array, or adding an eslint-disable comment if stability is guaranteed.Suggested approach
+ const { setExistingParticipants, setRoomId } = useGroupChatCreate() + useEffect(() => { if (group?.participantIds) { - groups.setExistingParticipants(group.participantIds.filter((id): id is string => id != null)) + setExistingParticipants(group.participantIds.filter((id): id is string => id != null)) } - groups.setRoomId(roomId) - }, [roomId, group?.participantIds]) + setRoomId(roomId) + }, [roomId, group?.participantIds, setExistingParticipants, setRoomId])
packages/components/modules/messages/common/context/GroupChatProvider/types.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/components/modules/messages/native/GroupDetailsPage/Members/type.ts (1)
23-23:⚠️ Potential issue | 🔴 CriticalMake
roomIdrequired in the props contract.Line 23 allows
roomIdto be omitted, but downstream navigation uses it as required path data, which can still produce/edit-group-details/undefined/add-members.Proposed fix
- roomId?: string + roomId: stringAlso ensure the parent passes
roomIdwhen rendering<Members />.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/components/modules/messages/native/GroupDetailsPage/Members/type.ts` at line 23, Update the props contract so roomId is required: change the optional field declaration roomId?: string in the Members prop/type definition (in packages/components/modules/messages/native/GroupDetailsPage/Members/type.ts) to a required roomId: string, and then ensure any parent that renders the <Members /> component passes a valid roomId prop (search for usages of <Members /> and add or forward the roomId value where missing) so navigation no longer constructs paths with undefined.
🧹 Nitpick comments (1)
packages/components/modules/messages/native/GroupDetailsPage/Members/index.tsx (1)
50-64: Extract a shared add-member navigation handler.The same
router.pushblock appears twice. A single handler reduces drift and gives one place to add route guards.Refactor sketch
+ const handleAddMemberPress = () => { + router.push(`/edit-group-details/${roomId}/add-members`) + } ... - <FabButton - onPress={() => { - router.push(`/edit-group-details/${roomId}/add-members`) - }} + <FabButton + onPress={handleAddMemberPress} ... - <Text + <Text variant="subtitle2" color="high" - onPress={() => { - router.push(`/edit-group-details/${roomId}/add-members`) - }} + onPress={handleAddMemberPress} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/components/modules/messages/native/GroupDetailsPage/Members/index.tsx` around lines 50 - 64, Extract the duplicate router.push(`/edit-group-details/${roomId}/add-members`) into a single handler (e.g., navigateToAddMembers or handleAddMembersNavigate) defined in the Members component and replace both onPress callbacks on the FabButton and the Text with that handler; make it a stable function (useCallback) so you can later add route guards or logging in one place and ensure it references the existing roomId and router symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/components/modules/messages/native/GroupDetailsPage/index.tsx`:
- Around line 54-59: The effect in GroupDetailsPage keeps stale participants
when group?.participantIds is undefined; update the useEffect that calls
groups.setExistingParticipants and groups.setRoomId so that when
group?.participantIds is falsy you explicitly reset the seed state by calling
groups.setExistingParticipants([]) (instead of leaving the previous list), and
continue to set groups.setRoomId(roomId); reference the useEffect block and the
functions groups.setExistingParticipants, groups.setRoomId, and the
group?.participantIds check when making this change.
---
Duplicate comments:
In
`@packages/components/modules/messages/native/GroupDetailsPage/Members/type.ts`:
- Line 23: Update the props contract so roomId is required: change the optional
field declaration roomId?: string in the Members prop/type definition (in
packages/components/modules/messages/native/GroupDetailsPage/Members/type.ts) to
a required roomId: string, and then ensure any parent that renders the <Members
/> component passes a valid roomId prop (search for usages of <Members /> and
add or forward the roomId value where missing) so navigation no longer
constructs paths with undefined.
---
Nitpick comments:
In
`@packages/components/modules/messages/native/GroupDetailsPage/Members/index.tsx`:
- Around line 50-64: Extract the duplicate
router.push(`/edit-group-details/${roomId}/add-members`) into a single handler
(e.g., navigateToAddMembers or handleAddMembersNavigate) defined in the Members
component and replace both onPress callbacks on the FabButton and the Text with
that handler; make it a stable function (useCallback) so you can later add route
guards or logging in one place and ensure it references the existing roomId and
router symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ab0c8cc-6dd4-49a1-93fe-764d147552c9
📒 Files selected for processing (7)
packages/components/modules/messages/native/GroupDetailsPage/Members/index.tsxpackages/components/modules/messages/native/GroupDetailsPage/Members/type.tspackages/components/modules/messages/native/GroupDetailsPage/index.tsxpackages/components/modules/messages/native/index.tspackages/components/modules/navigations/web/Header/AccountMenu/__shared__/PopoverContent/types.tspackages/components/modules/navigations/web/__shared__/NavToggleButton/types.tspackages/components/schema.graphql
| useEffect(() => { | ||
| if (group?.participantIds) { | ||
| groups.setExistingParticipants(group.participantIds.filter((id): id is string => id != null)) | ||
| } | ||
| groups.setRoomId(roomId) | ||
| }, [roomId, group?.participantIds]) |
There was a problem hiding this comment.
Reset participant seed state when participant IDs are unavailable.
The current effect keeps previous existingParticipants until data arrives. That can leak stale members between rooms.
Proposed fix
useEffect(() => {
- if (group?.participantIds) {
- groups.setExistingParticipants(group.participantIds.filter((id): id is string => id != null))
- }
+ const participantIds = group?.participantIds?.filter((id): id is string => id != null) ?? []
+ groups.setExistingParticipants(participantIds)
groups.setRoomId(roomId)
}, [roomId, group?.participantIds])📝 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.
| useEffect(() => { | |
| if (group?.participantIds) { | |
| groups.setExistingParticipants(group.participantIds.filter((id): id is string => id != null)) | |
| } | |
| groups.setRoomId(roomId) | |
| }, [roomId, group?.participantIds]) | |
| useEffect(() => { | |
| const participantIds = group?.participantIds?.filter((id): id is string => id != null) ?? [] | |
| groups.setExistingParticipants(participantIds) | |
| groups.setRoomId(roomId) | |
| }, [roomId, group?.participantIds]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/components/modules/messages/native/GroupDetailsPage/index.tsx`
around lines 54 - 59, The effect in GroupDetailsPage keeps stale participants
when group?.participantIds is undefined; update the useEffect that calls
groups.setExistingParticipants and groups.setRoomId so that when
group?.participantIds is falsy you explicitly reset the seed state by calling
groups.setExistingParticipants([]) (instead of leaving the previous list), and
continue to set groups.setRoomId(roomId); reference the useEffect block and the
functions groups.setExistingParticipants, groups.setRoomId, and the
group?.participantIds check when making this change.
|
There was a problem hiding this comment.
♻️ Duplicate comments (4)
packages/components/modules/messages/native/EditGroupChatDetails/index.tsx (3)
115-115:⚠️ Potential issue | 🟠 MajorGroup name cannot be cleared due truthy fallback.
Line 115treats''as falsy and falls back toexistingTitle, so users cannot clear the field once a title exists.Proposed fix
- value={groups.title ? groups.title : (existingTitle ?? '')} + value={groups.title !== undefined ? groups.title : (existingTitle ?? '')}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/components/modules/messages/native/EditGroupChatDetails/index.tsx` at line 115, The value prop currently uses a truthy check (value={groups.title ? groups.title : (existingTitle ?? '')}) which treats an empty string as falsy and prevents clearing the group name; change this to use a nullish check so an explicit empty string is preserved, e.g. replace that expression with groups.title ?? existingTitle ?? '' in the EditGroupChatDetails component's value prop so undefined/null fall back to existingTitle but '' remains writable.
45-48:⚠️ Potential issue | 🟡 MinorRemove debug log and resolve the TODO before merge.
Line 47still has a debugconsole.log, andLine 46has an unresolved TODO currently flagged by static analysis.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/components/modules/messages/native/EditGroupChatDetails/index.tsx` around lines 45 - 48, The handleSheetChanges function currently contains a TODO and a debug console.log; remove the console.log and implement actual behavior: replace the TODO by updating the component state or invoking the appropriate prop/callback when the sheet index changes (e.g., call setSheetIndex(index) if you have local state, or call a passed-in onSheetChange/onIndexChange prop) and ensure any required imports/hooks (useState or the prop) are used; update the function handleSheetChanges to perform that state update or callback and remove the TODO comment.
53-56:⚠️ Potential issue | 🔴 CriticalDo not spread the entire store into
setGroupChat.At
Line 55andLine 114,{ ...groups }includes action functions and non-form fields in update payloads.Proposed fix
- groups.setGroupChat({ ...groups, image: image.uri }) + groups.setGroupChat({ image: image.uri }) ... - onChangeText={(text) => groups.setGroupChat({ ...groups, title: text })} + onChangeText={(text) => groups.setGroupChat({ title: text })}Also applies to: 114-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/components/modules/messages/native/EditGroupChatDetails/index.tsx` around lines 53 - 56, You're spreading the entire `groups` store into `setGroupChat`, which pulls actions/non-form fields into the payload; change both call sites to update only the group's data by using a functional update or spreading the existing group object instead of the store — e.g., replace `groups.setGroupChat({ ...groups, image: image.uri })` with a safe update such as `groups.setGroupChat(prev => ({ ...prev, image: image.uri }))` (and apply the same fix at the other occurrence) so only the form/data fields are merged, not store methods.packages/components/modules/messages/common/context/GroupChatProvider/index.tsx (1)
21-22:⚠️ Potential issue | 🟠 Major
resetGroupChatcan preserve staleroomIdstate.With Zustand shallow merge,
Line 22only clears keys present inINITIAL_GROUP_CHAT_STATE. IfroomIdis not in that initial object, previous room context can leak into later flows.Proposed fix
// packages/components/modules/messages/common/context/GroupChatProvider/constants.ts export const INITIAL_GROUP_CHAT_STATE = { id: undefined, participants: [], existingParticipants: [], image: undefined, title: '', + roomId: undefined, }#!/bin/bash set -euo pipefail sed -n '1,120p' packages/components/modules/messages/common/context/GroupChatProvider/constants.ts sed -n '1,120p' packages/components/modules/messages/common/context/GroupChatProvider/index.tsx🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/components/modules/messages/common/context/GroupChatProvider/index.tsx` around lines 21 - 22, resetGroupChat currently spreads INITIAL_GROUP_CHAT_STATE which doesn't clear keys not present there (like roomId), allowing stale roomId to persist; change resetGroupChat to clear roomId explicitly—e.g. have resetGroupChat call set({ ...INITIAL_GROUP_CHAT_STATE, roomId: undefined }) (or use the Zustand replace form set(() => INITIAL_GROUP_CHAT_STATE, true)) so roomId cannot leak after reset; update the resetGroupChat implementation accordingly in the function where setRoomId and resetGroupChat are defined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@packages/components/modules/messages/common/context/GroupChatProvider/index.tsx`:
- Around line 21-22: resetGroupChat currently spreads INITIAL_GROUP_CHAT_STATE
which doesn't clear keys not present there (like roomId), allowing stale roomId
to persist; change resetGroupChat to clear roomId explicitly—e.g. have
resetGroupChat call set({ ...INITIAL_GROUP_CHAT_STATE, roomId: undefined }) (or
use the Zustand replace form set(() => INITIAL_GROUP_CHAT_STATE, true)) so
roomId cannot leak after reset; update the resetGroupChat implementation
accordingly in the function where setRoomId and resetGroupChat are defined.
In `@packages/components/modules/messages/native/EditGroupChatDetails/index.tsx`:
- Line 115: The value prop currently uses a truthy check (value={groups.title ?
groups.title : (existingTitle ?? '')}) which treats an empty string as falsy and
prevents clearing the group name; change this to use a nullish check so an
explicit empty string is preserved, e.g. replace that expression with
groups.title ?? existingTitle ?? '' in the EditGroupChatDetails component's
value prop so undefined/null fall back to existingTitle but '' remains writable.
- Around line 45-48: The handleSheetChanges function currently contains a TODO
and a debug console.log; remove the console.log and implement actual behavior:
replace the TODO by updating the component state or invoking the appropriate
prop/callback when the sheet index changes (e.g., call setSheetIndex(index) if
you have local state, or call a passed-in onSheetChange/onIndexChange prop) and
ensure any required imports/hooks (useState or the prop) are used; update the
function handleSheetChanges to perform that state update or callback and remove
the TODO comment.
- Around line 53-56: You're spreading the entire `groups` store into
`setGroupChat`, which pulls actions/non-form fields into the payload; change
both call sites to update only the group's data by using a functional update or
spreading the existing group object instead of the store — e.g., replace
`groups.setGroupChat({ ...groups, image: image.uri })` with a safe update such
as `groups.setGroupChat(prev => ({ ...prev, image: image.uri }))` (and apply the
same fix at the other occurrence) so only the form/data fields are merged, not
store methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da60dd79-78c7-4134-9842-245564c304c6
📒 Files selected for processing (4)
packages/components/modules/messages/common/context/GroupChatProvider/index.tsxpackages/components/modules/messages/common/context/GroupChatProvider/types.tspackages/components/modules/messages/native/EditGroupChatDetails/index.tsxpackages/components/modules/messages/native/GroupDetailsPage/Members/index.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/components/modules/messages/native/GroupDetailsPage/Members/index.tsx
- packages/components/modules/messages/common/context/GroupChatProvider/types.ts
| setRoomId: (roomId: string) => set({ roomId }), | ||
| resetGroupChat: () => set({ ...INITIAL_GROUP_CHAT_STATE }), |
There was a problem hiding this comment.
@rf2tsl what setRoomId does here? since we dont have roomId in the defaults. Isn't this method the same as setGroupChat?
| groups.setExistingParticipants(group.participantIds.filter((id): id is string => id != null)) | ||
| } | ||
| groups.setRoomId(roomId) | ||
| }, [roomId, group?.participantIds]) |
There was a problem hiding this comment.
I wonder if we could do a more event-driven approach, like every time we change the participantIds using setParticipants, we automatically filter and set the existingParticipants? Would be nice to avoid the useeffect here
There was a problem hiding this comment.
we may need the existing participants before any event takes so this change is hard to make without the useEffect, I have tried setting the existing participants when the user clicks 'add member' but it doesn't work.
2 participants
Acceptance Criteria
Scenario: Edit group details
Given I am on the Group Details page when I tap the pencil icon (edit button)
then I should be navigated to the Edit Group Details page and I should be able to:
Change the group name
Change or remove the group avatar
Scenario: Add or remove members from group
Given I am on the Group Details page when I tap "Add Member" then I should be navigated to the Member Settings page and I should see a list of current members with Checked mark in grey and with and empty Checked space for the users that aren't in the group.
When I tap "Add" next to a contact then that contact should be marked for addition in the top as New Members, and when I tap the confirmation checkmark, then the group should update to reflect the added.
Design Link: https://www.figma.com/design/z5ZdVlKEMmGIX4GVnjwfxA/BaseApp---NATIVE?node-id=7829-37847&t=qZVBT6H3IAyD7hH8-0
Approvd
https://app.approvd.io/silverlogic/BA/stories/42178
Summary by CodeRabbit
New Features
Chores