Conversation
rf2tsl
requested review from
anicioalexandre and
priscilladeroode
as code owners
|
📝 WalkthroughWalkthroughCentralizes group-form constants/types into Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (UI)
participant Edit as EditGroupChatDetails (Component)
participant Picker as ImagePicker
participant Context as GroupChatProvider (useGroupChatCreate)
participant Relay as Relay (GroupDetailsQuery/Mutation)
User->>Edit: open edit screen (roomId)
Edit->>Relay: fetch GroupDetailsQuery(roomId)
Relay-->>Edit: return room data (title,image,participantIds)
Edit->>Context: setExistingParticipants(participantIds), setRoomId(roomId)
User->>Edit: choose "Change Avatar"
Edit->>Picker: open image picker
Picker-->>Edit: selected image/file
Edit->>Context: set groups.image (new File/URI)
User->>Edit: save
Edit->>Relay: send UpdateChatRoom mutation (with 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 | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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
| [CREATE_OR_EDIT_GROUP_FORM_VALUE.title]: z | ||
| .string() | ||
| .min(1, { message: 'Please enter a title' }) | ||
| .max(20, { message: "Title can't be more than 20 characters" }), |
There was a problem hiding this comment.
Title max-length mismatch: validation says 20, but the native TextInput allows 30.
The zod schema here caps the title at 20 characters, while EditGroupChatDetails/index.tsx Line 95 sets maxLength={30} on the <TextInput>. Users can type up to 30 characters but validation will reject anything over 20—confusing UX.
Align both to the same limit.
🤖 Prompt for AI Agents
In `@packages/components/modules/messages/common/constants.ts` at 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.
| 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.
| const handleSheetChanges = useCallback((index: number) => { | ||
| // TODO: handle sheet changes | ||
| console.log('index', index) | ||
| }, []) |
There was a problem hiding this comment.
Remove console.log debug artifact before merging.
Proposed fix
const handleSheetChanges = useCallback((index: number) => {
- // TODO: handle sheet changes
- console.log('index', index)
+ // TODO: handle sheet changes
}, [])📝 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.
| const handleSheetChanges = useCallback((index: number) => { | |
| // TODO: handle sheet changes | |
| console.log('index', index) | |
| }, []) | |
| const handleSheetChanges = useCallback((index: number) => { | |
| // TODO: handle sheet changes | |
| }, []) |
🤖 Prompt for AI Agents
In `@packages/components/modules/messages/native/EditGroupChatDetails/index.tsx`
around lines 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.
| const { handleViewPhotoLibrary, handleTakePhoto } = useViewPhotoLibrary({ | ||
| allowsEditing: Platform.OS === 'android', | ||
| isBase64: false, | ||
| onResult: (image?: ImagePicker.ImagePickerAsset) => { | ||
| if (image?.uri) { | ||
| groups.setGroupChat({ ...groups, image: image.uri }) | ||
| } | ||
| }, | ||
| onFinally: () => { | ||
| bottomDrawerRef.current?.close() | ||
| }, | ||
| type: 'image', | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the file
find . -path "*/EditGroupChatDetails/index.tsx" -type fRepository: silverlogic/baseapp-frontend
Length of output: 148
🏁 Script executed:
# Also search for the file using git ls-files
git ls-files | grep -i "EditGroupChatDetails"Repository: silverlogic/baseapp-frontend
Length of output: 295
🏁 Script executed:
# Search for useGroupChatCreate to find the store definition
rg -l "useGroupChatCreate" --type=ts --type=tsxRepository: silverlogic/baseapp-frontend
Length of output: 99
🏁 Script executed:
# Read the EditGroupChatDetails/index.tsx file
cat -n ./packages/components/modules/messages/native/EditGroupChatDetails/index.tsxRepository: silverlogic/baseapp-frontend
Length of output: 5092
🏁 Script executed:
# Search for useGroupChatCreate definition (without tsx type specifier)
rg -n "useGroupChatCreate" --type=ts -A 10Repository: silverlogic/baseapp-frontend
Length of output: 15960
🏁 Script executed:
# Read the GroupChatProvider to understand the store structure
cat -n ./packages/components/modules/messages/common/context/GroupChatProvider/index.tsxRepository: silverlogic/baseapp-frontend
Length of output: 1670
🏁 Script executed:
# Find and read the types and constants for GroupChatProvider
find ./packages/components/modules/messages/common/context/GroupChatProvider -type f -name "*.ts" -o -name "*.tsx"Repository: silverlogic/baseapp-frontend
Length of output: 319
🏁 Script executed:
# Read the directory structure of GroupChatProvider
ls -la ./packages/components/modules/messages/common/context/GroupChatProvider/Repository: silverlogic/baseapp-frontend
Length of output: 362
🏁 Script executed:
# Read the types file
cat -n ./packages/components/modules/messages/common/context/GroupChatProvider/types.tsRepository: silverlogic/baseapp-frontend
Length of output: 908
🏁 Script executed:
# Read the constants file
cat -n ./packages/components/modules/messages/common/context/GroupChatProvider/constants.tsRepository: silverlogic/baseapp-frontend
Length of output: 311
Bug: spreading groups (the entire zustand store) into setGroupChat leaks action functions into state.
groups is the full store object returned by useGroupChatCreate(), which includes state fields (id, participants, image, title, roomId) and action methods (setGroupChat, resetGroupChat, setRoomId, setParticipants). Spreading { ...groups, image: ... } passes all of these into the state update, including the action functions. setGroupChat expects only Partial<GroupChatCreateState>, not the full store interface.
Extract only the state fields you need:
Proposed fix
onResult: (image?: ImagePicker.ImagePickerAsset) => {
if (image?.uri) {
- groups.setGroupChat({ ...groups, image: image.uri })
+ groups.setGroupChat({ title: groups.title, image: image.uri })
}
}, const handleRemoveImage = () => {
if (existingImage && groups.image === undefined) {
- groups.setGroupChat({ ...groups, image: '' })
+ groups.setGroupChat({ title: groups.title, image: '' })
return
}
- groups.setGroupChat({ ...groups, image: undefined })
+ groups.setGroupChat({ title: groups.title, image: undefined })
}- onChangeText={(text) => groups.setGroupChat({ ...groups, title: text })}
+ onChangeText={(text) => groups.setGroupChat({ title: text, image: groups.image })}Also: Remove console.log('index', index) on line 47 (debug artifact).
📝 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.
| const { handleViewPhotoLibrary, handleTakePhoto } = useViewPhotoLibrary({ | |
| allowsEditing: Platform.OS === 'android', | |
| isBase64: false, | |
| onResult: (image?: ImagePicker.ImagePickerAsset) => { | |
| if (image?.uri) { | |
| groups.setGroupChat({ ...groups, image: image.uri }) | |
| } | |
| }, | |
| onFinally: () => { | |
| bottomDrawerRef.current?.close() | |
| }, | |
| type: 'image', | |
| }) | |
| const { handleViewPhotoLibrary, handleTakePhoto } = useViewPhotoLibrary({ | |
| allowsEditing: Platform.OS === 'android', | |
| isBase64: false, | |
| onResult: (image?: ImagePicker.ImagePickerAsset) => { | |
| if (image?.uri) { | |
| groups.setGroupChat({ title: groups.title, image: image.uri }) | |
| } | |
| }, | |
| onFinally: () => { | |
| bottomDrawerRef.current?.close() | |
| }, | |
| type: 'image', | |
| }) |
🤖 Prompt for AI Agents
In `@packages/components/modules/messages/native/EditGroupChatDetails/index.tsx`
around lines 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.
| label="Group Name" | ||
| placeholder="Group Name" | ||
| onChangeText={(text) => groups.setGroupChat({ ...groups, title: text })} | ||
| value={groups.title ? groups.title : (existingTitle ?? '')} |
There was a problem hiding this comment.
User cannot clear the group name — falsy check falls back to existingTitle.
groups.title ? groups.title : (existingTitle ?? '') uses a truthy check, so an empty string '' is falsy and reverts to existingTitle. This means once a group has a title, the user can never clear the input field.
Use an explicit undefined check instead:
Proposed fix
- value={groups.title ? groups.title : (existingTitle ?? '')}
+ value={groups.title !== undefined ? groups.title : (existingTitle ?? '')}📝 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.
| value={groups.title ? groups.title : (existingTitle ?? '')} | |
| value={groups.title !== undefined ? groups.title : (existingTitle ?? '')} |
🤖 Prompt for AI Agents
In `@packages/components/modules/messages/native/EditGroupChatDetails/index.tsx`
at 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.
| <FabButton | ||
| // TODO: implement add member functionality | ||
| onPress={() => {}} | ||
| onPress={() => { | ||
| router.push(`/edit-group-details/${roomId}/add-members`) | ||
| }} |
There was a problem hiding this comment.
roomId is never passed from the parent GroupDetailsPage, so navigation will break.
Looking at GroupDetailsPage/index.tsx Lines 98–104, <Members> is rendered without the roomId prop. Since roomId is typed as optional (roomId?: string), this won't cause a type error, but the navigation will route to /edit-group-details/undefined/add-members.
Either make roomId required in MembersProps and pass it from the parent, or add a guard here:
Proposed fix in GroupDetailsPage/index.tsx (parent)
<Members
participantsCount={group?.participantsCount}
members={members}
loadNext={handleLoadMoreMembers}
isLoadingNext={isLoadingNext}
hasNext={hasNext}
+ roomId={roomId}
/>🤖 Prompt for AI Agents
In
`@packages/components/modules/messages/native/GroupDetailsPage/Members/index.tsx`
around lines 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.
|
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])
| export type GroupChatCreateState = { | ||
| id?: string | ||
| participants?: ProfileItemFragment$data[] | ||
| existingParticipants?: String[] |
There was a problem hiding this comment.
Use lowercase string instead of String.
String is the JavaScript wrapper object type; the TypeScript primitive is string. Using String is a known anti-pattern that can cause subtle type incompatibilities (e.g., string is not assignable to String in strict checks). Lines 9 and 21 already use string correctly — these two should be consistent.
Proposed fix
- existingParticipants?: String[]
+ existingParticipants?: string[]- setExistingParticipants: (existingParticipants: String[]) => void
+ setExistingParticipants: (existingParticipants: string[]) => voidAlso applies to: 14-14
🤖 Prompt for AI Agents
In
`@packages/components/modules/messages/common/context/GroupChatProvider/types.ts`
at 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).
1 participant
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