Conversation
WalkthroughAdded a guard in group creation to avoid updating inclusion when toggling iAmIncluded true without a primary wallet, moved the iAmIncluded update after that check, and tightened the condition that appends the primary wallet to identity_addresses to require both newState true and a primaryWallet. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as GroupCreate UI
participant State as Local State
participant Wallet as PrimaryWalletService
Note over UI,State: User toggles "I am included"
UI->>Wallet: request primaryWallet (may be null)
alt no primaryWallet and toggling ON
Wallet-->>UI: null
UI->>State: early return (no iAmIncluded update)
Note right of State: no wallet appended
else primaryWallet exists or toggling OFF
Wallet-->>UI: primaryWallet
UI->>State: set iAmIncluded (update placed after guard)
alt newState true and primaryWallet exists
UI->>State: append primaryWallet to identity_addresses
else newState false
UI->>State: remove primaryWallet from identity_addresses (if present)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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: 0
🧹 Nitpick comments (1)
components/groups/page/create/GroupCreate.tsx (1)
188-190: Optional: Simplify redundant condition.The
primaryWalletcheck here is redundant because the early return at lines 178-180 already guarantees that ifnewStateistrue,primaryWalletmust exist. You could simplify this toif (newState).However, the current form is more defensive and explicit, which may be preferable for clarity.
- if (newState && primaryWallet) { + if (newState) { newAddresses.push(primaryWallet); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/groups/page/create/GroupCreate.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: Do not include any comments in the code
Use react-query for data fetching
Always add readonly before propsUse TypeScript across the codebase
Files:
components/groups/page/create/GroupCreate.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use FontAwesome for icons
Use TailwindCSS for stylingUse React functional components with hooks for UI components
Files:
components/groups/page/create/GroupCreate.tsx
⏰ 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). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
components/groups/page/create/GroupCreate.tsx (1)
178-181: LGTM! Good defensive guard.The early return when attempting to include the user without a primary wallet prevents invalid state updates. The placement of
setIAmIncludedafter this guard ensures the state is only updated when it's safe to proceed.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
__tests__/components/utils/select/dropdown/CommonDropdownItem.test.tsx (2)
16-16: Good change to menuitem role, but consider using userEvent.The update to target
menuitemrole improves accessibility and semantic correctness.However, the test uses
fireEventinstead ofuserEventfrom@testing-library/user-event, which deviates from the coding guidelines.userEventmore closely simulates real user interactions and is the recommended approach.Apply this diff to use
userEvent:-import { render, screen, fireEvent, act } from '@testing-library/react'; +import { render, screen, act } from '@testing-library/react'; +import userEvent from '@testing-library/user-event';Then update the test:
- fireEvent.click(screen.getByRole('menuitem', { name: 'Item' })); + await userEvent.click(screen.getByRole('menuitem', { name: 'Item' }));Note: You'll need to make the test async:
-test('calls setSelected on click', () => { +test('calls setSelected on click', async () => {As per coding guidelines.
25-25: Menuitem role improves test reliability.The change to
menuitemrole correctly reflects the component's accessible structure and makes the test more robust.The use of
querySelector('svg')works but is less maintainable than@testing-libraryqueries. Consider adding adata-testidor usinggetByRoleif the SVG has an accessible role.__tests__/components/utils/input/identity/IdentitySearch.test.tsx (1)
20-31: Consider migrating fromfireEventtouserEventfor more realistic interactions.As per coding guidelines, React component tests should use
@testing-library/user-eventinstead offireEvent. This provides more realistic user interaction simulation.Apply this refactor:
-import { render, screen, fireEvent } from '@testing-library/react'; +import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event';Then update the test:
- it('opens dropdown after typing and selects value', () => { + it('opens dropdown after typing and selects value', async () => { + const user = userEvent.setup(); render(<IdentitySearch identity={null} setIdentity={setIdentity} />); const input = screen.getByRole('combobox', { name: 'Identity' }); - fireEvent.focus(input); + await user.click(input); expect(receivedProps.open).toBe(false); - fireEvent.change(input, { target: { value: 'a' } }); + await user.type(input, 'a'); expect(receivedProps.open).toBe(false); - fireEvent.change(input, { target: { value: 'abc' } }); + await user.clear(input); + await user.type(input, 'abc'); expect(receivedProps.open).toBe(true); receivedProps.onProfileSelect({ handle: 'user' }); expect(setIdentity).toHaveBeenCalledWith('user'); });And update the second test similarly:
- it('clears identity when clear button clicked', () => { + it('clears identity when clear button clicked', async () => { + const user = userEvent.setup(); render(<IdentitySearch identity="bob" setIdentity={setIdentity} />); - fireEvent.click(screen.getByLabelText('Clear identity')); + await user.click(screen.getByLabelText('Clear identity')); expect(setIdentity).toHaveBeenCalledWith(null); });As per coding guidelines.
__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx (2)
66-74: Good mock structure; consider removing redundant module-level setup.The
queryClientMockis well-structured. However, lines 72-74 duplicate the mock configuration that's already done inbeforeEach(lines 130-132). SincebeforeEachruns before each test and reconfigures all mocks, the module-level setup is redundant.Consider removing the redundant module-level mock setup:
const queryClientMock = { ensureQueryData: jest.fn(), fetchQuery: jest.fn(), setQueryData: jest.fn(), }; - -(useMutation as jest.Mock).mockReturnValue({ mutateAsync }); -(useQuery as jest.Mock).mockReturnValue({ data: undefined }); -(useQueryClient as jest.Mock).mockImplementation(() => queryClientMock);
135-176: Consider usinguserEventinstead offireEvent.The tests currently use
fireEvent, which works but is lower-level. The coding guidelines specify using@testing-library/user-eventfor React component tests, which better simulates real user interactions including timing, event sequences, and focus management.As per coding guidelines.
Example refactor for one test:
+import userEvent from '@testing-library/user-event'; + describe('WaveGroupEditButtons', () => { beforeEach(() => { // ... }); it('opens menu and calls mutate on edit', async () => { + const user = userEvent.setup(); render(<WaveGroupEditButtons haveGroup wave={wave} type={WaveGroupType.VIEW} />, { wrapper }); - fireEvent.click(screen.getByRole('button', { name: /Group options/i })); - fireEvent.click(screen.getByText('Change group')); + await user.click(screen.getByRole('button', { name: /Group options/i })); + await user.click(screen.getByText('Change group')); await waitFor(() => expect(auth.requestAuth).toHaveBeenCalled()); expect(mutateAsync).toHaveBeenCalled(); });Apply similar changes to the remaining tests.
__tests__/components/waves/specs/groups/group/edit/buttons/useWaveGroupEditButtonsController.test.tsx (5)
39-43: Broaden queryClient mock surface to avoid brittle tests.Add common no‑ops like invalidateQueries and getQueryData to match v5 usage patterns and prevent future breakage.
const queryClientMock = { ensureQueryData: jest.fn(), fetchQuery: jest.fn(), setQueryData: jest.fn(), + invalidateQueries: jest.fn(), + getQueryData: jest.fn(), };
115-132: Prevent double execution of queryFn in useQuery mock.Calling queryFn inside useQuery plus ensureQueryData/fetchQuery can fetch twice and introduce flakiness. Return stable shape without side effects.
- (useQuery as jest.Mock).mockImplementation(({ enabled, queryFn }) => { - if (enabled && typeof queryFn === "function") { - void queryFn({ signal: undefined }); - } - return { data: undefined }; - }); + (useQuery as jest.Mock).mockImplementation(() => ({ + data: undefined, + isFetching: false, + isPending: false, + status: 'success', + }));
133-148: Augment useMutation mock to mirror v5 shape.Add mutate alias and status flags to better emulate @tanstack/react-query v5 and future‑proof dependent logic.
- (useMutation as jest.Mock).mockImplementation((options: any) => ({ - mutateAsync: async (params?: any) => { + (useMutation as jest.Mock).mockImplementation((options: any) => ({ + status: 'idle', + isPending: false, + mutate: (params?: any) => { + // fire-and-forget alias that calls mutateAsync under the hood + // errors are intentionally unhandled here to mirror real mutate + // eslint-disable-next-line @typescript-eslint/no-floating-promises + (async () => { await (useMutation as any).mock.results.at(-1).value.mutateAsync(params); })(); + }, + mutateAsync: async (params?: any) => { try { const result = await options.mutationFn(params); options.onSuccess?.(result, params, undefined); options.onSettled?.(result, undefined, params, undefined); mutateAsyncSpy(params); return result; } catch (error) { options.onError?.(error, params, undefined); options.onSettled?.(undefined, error, params, undefined); mutateAsyncSpy(params); throw error; } }, }));
169-175: DRY up repeated 'groups/new-group-id' endpoint stubs.Extract a small helper to register this common case across tests to reduce duplication.
+const mockFetchNewGroup = () => ({ + ...baseGroupFull, + id: 'new-group-id', + visible: true, +}); ... - if (endpoint === 'groups/new-group-id') { - return Promise.resolve({ - ...baseGroupFull, - id: 'new-group-id', - visible: true, - }); - } + if (endpoint === 'groups/new-group-id') return Promise.resolve(mockFetchNewGroup());Also applies to: 279-285, 325-331
218-229: New “no scoped group” path covered — add one more payload assertion.Great coverage of create→publish flow and toast. Optionally assert that identity casing is normalized in the payload to catch regressions.
+ const payloadArg = mockCreateGroup.mock.calls[0][0].payload; + expect(payloadArg.group.identity_addresses).toContain('0xface');Also applies to: 242-242, 251-265
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
__tests__/components/common/TabToggleWithOverflow.test.tsx(1 hunks)__tests__/components/meme-calendar/meme-calendar.helpers.test.ts(1 hunks)__tests__/components/utils/input/identity/IdentitySearch.test.tsx(1 hunks)__tests__/components/utils/input/profile-search/CommonProfileSearchItem.test.tsx(1 hunks)__tests__/components/utils/input/profile-search/CommonProfileSearchItems.test.tsx(1 hunks)__tests__/components/utils/select/dropdown/CommonDropdownItem.test.tsx(2 hunks)__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx(3 hunks)__tests__/components/waves/specs/groups/group/edit/buttons/useWaveGroupEditButtonsController.test.tsx(10 hunks)codex/tickets/TKT-0012.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- codex/tickets/TKT-0012.md
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: Do not include any comments in the code
Use react-query for data fetching
Always add readonly before propsUse TypeScript across the codebase
Files:
__tests__/components/utils/input/profile-search/CommonProfileSearchItems.test.tsx__tests__/components/meme-calendar/meme-calendar.helpers.test.ts__tests__/components/utils/input/profile-search/CommonProfileSearchItem.test.tsx__tests__/components/common/TabToggleWithOverflow.test.tsx__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx__tests__/components/utils/select/dropdown/CommonDropdownItem.test.tsx__tests__/components/waves/specs/groups/group/edit/buttons/useWaveGroupEditButtonsController.test.tsx__tests__/components/utils/input/identity/IdentitySearch.test.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use FontAwesome for icons
Use TailwindCSS for stylingUse React functional components with hooks for UI components
Files:
__tests__/components/utils/input/profile-search/CommonProfileSearchItems.test.tsx__tests__/components/utils/input/profile-search/CommonProfileSearchItem.test.tsx__tests__/components/common/TabToggleWithOverflow.test.tsx__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx__tests__/components/utils/select/dropdown/CommonDropdownItem.test.tsx__tests__/components/waves/specs/groups/group/edit/buttons/useWaveGroupEditButtonsController.test.tsx__tests__/components/utils/input/identity/IdentitySearch.test.tsx
__tests__/**
📄 CodeRabbit inference engine (tests/AGENTS.md)
Place Jest test suites under the
__tests__directory mirroring source folders (e.g., components, contexts, hooks, utils)
Files:
__tests__/components/utils/input/profile-search/CommonProfileSearchItems.test.tsx__tests__/components/meme-calendar/meme-calendar.helpers.test.ts__tests__/components/utils/input/profile-search/CommonProfileSearchItem.test.tsx__tests__/components/common/TabToggleWithOverflow.test.tsx__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx__tests__/components/utils/select/dropdown/CommonDropdownItem.test.tsx__tests__/components/waves/specs/groups/group/edit/buttons/useWaveGroupEditButtonsController.test.tsx__tests__/components/utils/input/identity/IdentitySearch.test.tsx
__tests__/components/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (tests/AGENTS.md)
Use
@testing-library/reactand@testing-library/user-eventfor React component tests
Files:
__tests__/components/utils/input/profile-search/CommonProfileSearchItems.test.tsx__tests__/components/meme-calendar/meme-calendar.helpers.test.ts__tests__/components/utils/input/profile-search/CommonProfileSearchItem.test.tsx__tests__/components/common/TabToggleWithOverflow.test.tsx__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx__tests__/components/utils/select/dropdown/CommonDropdownItem.test.tsx__tests__/components/waves/specs/groups/group/edit/buttons/useWaveGroupEditButtonsController.test.tsx__tests__/components/utils/input/identity/IdentitySearch.test.tsx
**/__tests__/**
📄 CodeRabbit inference engine (AGENTS.md)
Place tests in
__tests__directories when organizing test suites
Files:
__tests__/components/utils/input/profile-search/CommonProfileSearchItems.test.tsx__tests__/components/meme-calendar/meme-calendar.helpers.test.ts__tests__/components/utils/input/profile-search/CommonProfileSearchItem.test.tsx__tests__/components/common/TabToggleWithOverflow.test.tsx__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx__tests__/components/utils/select/dropdown/CommonDropdownItem.test.tsx__tests__/components/waves/specs/groups/group/edit/buttons/useWaveGroupEditButtonsController.test.tsx__tests__/components/utils/input/identity/IdentitySearch.test.tsx
**/*.test.tsx
📄 CodeRabbit inference engine (AGENTS.md)
When colocating tests with components, name them
ComponentName.test.tsx
Files:
__tests__/components/utils/input/profile-search/CommonProfileSearchItems.test.tsx__tests__/components/utils/input/profile-search/CommonProfileSearchItem.test.tsx__tests__/components/common/TabToggleWithOverflow.test.tsx__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx__tests__/components/utils/select/dropdown/CommonDropdownItem.test.tsx__tests__/components/waves/specs/groups/group/edit/buttons/useWaveGroupEditButtonsController.test.tsx__tests__/components/utils/input/identity/IdentitySearch.test.tsx
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Mock external dependencies and APIs in tests
Files:
__tests__/components/utils/input/profile-search/CommonProfileSearchItems.test.tsx__tests__/components/meme-calendar/meme-calendar.helpers.test.ts__tests__/components/utils/input/profile-search/CommonProfileSearchItem.test.tsx__tests__/components/common/TabToggleWithOverflow.test.tsx__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx__tests__/components/utils/select/dropdown/CommonDropdownItem.test.tsx__tests__/components/waves/specs/groups/group/edit/buttons/useWaveGroupEditButtonsController.test.tsx__tests__/components/utils/input/identity/IdentitySearch.test.tsx
🧬 Code graph analysis (2)
__tests__/components/utils/input/profile-search/CommonProfileSearchItems.test.tsx (1)
components/utils/input/profile-search/CommonProfileSearchItems.tsx (1)
CommonProfileSearchItems(7-159)
__tests__/components/utils/input/profile-search/CommonProfileSearchItem.test.tsx (1)
components/utils/input/profile-search/CommonProfileSearchItem.tsx (1)
CommonProfileSearchItem(7-88)
⏰ 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). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (14)
__tests__/components/utils/input/identity/IdentitySearch.test.tsx (2)
22-22: LGTM! Improved accessibility testing.Using
getByRole('combobox', { name: 'Identity' })is a more semantic and accessible way to query the input element, ensuring the component properly exposes ARIA roles and labels.
26-28: LGTM! Proper dropdown state verification.The assertions correctly verify that the dropdown remains closed after typing a single character ('a') and only opens after typing three characters ('abc'), ensuring the minimum character threshold is enforced.
__tests__/components/utils/input/profile-search/CommonProfileSearchItems.test.tsx (1)
27-27: Verify that these test changes belong in this PR.The test assertions have been correctly updated to expect 2 occurrences of each message (one in the screen-reader
<select>and one in the visible<li>), which accurately reflects the component's dual-rendering for accessibility. However, these profile search test changes appear unrelated to the PR objectives, which describe group creation andiAmIncludedguard logic.Please confirm whether these test updates are intentional for this PR or if they should be moved to a separate PR focused on profile search improvements.
Also applies to: 32-32
__tests__/components/utils/input/profile-search/CommonProfileSearchItem.test.tsx (2)
9-16: LGTM: Correct semantic HTML structure.Wrapping the
<CommonProfileSearchItem>in a<ul>element is semantically correct, as<li>elements must be children of list elements. This ensures valid HTML structure during testing.
18-26: LGTM: Test accurately reflects component implementation.The test updates correctly match the component's actual behavior:
- Alt text
"Alice avatar"matches the implementation'savatarAltTextconstruction- Selecting by text and then finding the closest
<li>correctly targets the component's root element- Asserting
data-option-idattribute verifies proper ID assignment- SVG assertion confirms the checkmark icon renders when selected
- Clicking the list item directly is correct, as the
onClickhandler is attached to the<li>elementNote: Like the previous test file, these profile search changes appear unrelated to the PR's stated objectives about group creation logic.
__tests__/components/common/TabToggleWithOverflow.test.tsx (4)
93-94: Consider restoring keyboard navigation if possible.Replacing
await user.tab()withmoreButton.focus()reduces test coverage of keyboard navigation to reach the button. While programmatic focus may be necessary to fix flaky behavior, it's less representative of real user interaction.Verify whether:
- The tab navigation was causing test flakiness
- The component's focus management changed
- Keyboard navigation to the button is tested elsewhere
If focus issues have been resolved, consider restoring the original approach:
- moreButton.focus(); - expect(moreButton).toHaveFocus(); + await user.tab(); // focus More tabs button
96-99: LGTM! Excellent async handling.Wrapping the aria-expanded assertion in
waitForand usingawait screen.findByRoleproperly handles the asynchronous menu rendering. This prevents race conditions and makes the test more robust.
104-108: LGTM! Proper async assertions for menu closing.Using separate
waitForblocks for the aria-expanded state change and menu item removal ensures both the state update and DOM cleanup complete before proceeding. This makes the test more reliable.
111-113: LGTM! Consistent async handling.Wrapping the aria-expanded assertion in
waitFormaintains consistency with the earlier Enter key test and properly handles the asynchronous state change when reopening the menu with the Space key.__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx (3)
7-13: LGTM! Comprehensive React Query mock coverage.The expanded mocks for
useQueryanduseQueryClientenable better test coverage of query-dependent scenarios.
17-28: LGTM! Proper ref forwarding implementation.The
forwardRefpattern correctly handles both callback refs and object refs, enabling parent components to imperatively trigger theopenhandler. The implementation is React 19 compatible.Also applies to: 33-44
123-132: LGTM! Thorough test isolation setup.The
beforeEachcomprehensively resets all mocks and establishes a clean, predictable state for each test. This ensures proper test isolation.__tests__/components/waves/specs/groups/group/edit/buttons/useWaveGroupEditButtonsController.test.tsx (2)
203-208: Correct assertion for publish payload (id + oldVersionId).Solid check that publish uses the recreated group id and previous version id.
8-8: React Query mocks extended — verified and approved.Jest alias mapping for
@/is properly configured injest.config.jswith"^@/(.*)$": "<rootDir>/$1", so the imports will resolve correctly. Module mock hoisting is standard Jest behavior and will apply before module evaluation. The test setup looks good.
2 participants
Summary by CodeRabbit
Bug Fixes
Tests