Inline group creating when editing wave#2285
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 31 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis refactor removes externally managed inline group-builder state from useWaveConfig and component props, moves builder state into CreateWaveGroupInlinePanel, breaks the group search into a hook + input/results components, and adds a WaveGroupChangeDialog and associated change/create flows with updated auth and mutation handling. Changes
Sequence Diagram(s)sequenceDiagram
actor User as "User"
rect rgba(240,248,255,0.5)
participant UI as "CreateWaveGroupInlinePanel / Dialog"
participant SearchHook as "useCreateWaveGroupSearch"
participant API as "Groups API"
end
rect rgba(245,255,250,0.5)
participant Mutations as "useGroupMutations / updateWave"
participant AppState as "Parent (onChange/onCreateGroup)"
end
User->>UI: open panel / type search
UI->>SearchHook: onInputChange / onInputFocus
SearchHook->>API: query groups (debounced)
API-->>SearchHook: suggestions
SearchHook-->>UI: suggestions / open list
User->>UI: select suggestion
UI->>AppState: onChange(selectedGroup)
AppState-->>UI: propagate selection (selectedGroup)
User->>UI: click create new group
UI->>Mutations: onCreateGroup(payload)
Mutations->>API: submit create
API-->>Mutations: created group
Mutations-->>AppState: return created group
AppState->>UI: onChange(createdGroup)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
__tests__/components/waves/create-wave/groups/CreateWaveGroupInlinePanel.test.tsx (1)
45-69:⚠️ Potential issue | 🟠 MajorMake the search-field mock respect
allowClear.The real
CreateWaveGroupSearchFieldnow gates clearing behindallowClear, but this mock always renders a working"clear group"button. That means the new "clearing is disabled" test is no longer verifying the real integration and can pass or fail for the wrong reason. Mirror the prop in the mock before asserting disabled-clear behavior.🧪 Suggested fix
<div data-testid="group-search"> <div>{props.selectedGroup?.name ?? "No group selected"}</div> <button type="button" onClick={() => props.onSelect({ id: "group-2", name: "Selected Group", created_by: { handle: "builder" }, }) } > select group </button> - <button type="button" onClick={() => props.onSelect(null)}> - clear group - </button> + {props.allowClear !== false && ( + <button type="button" onClick={() => props.onSelect(null)}> + clear group + </button> + )} </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/waves/create-wave/groups/CreateWaveGroupInlinePanel.test.tsx` around lines 45 - 69, The mock for CreateWaveGroupSearchField always renders an active "clear group" button which ignores the allowClear prop; update the MockCreateWaveGroupSearchField implementation to read props.allowClear and only render the clear-control when allowClear is true (or render a disabled/absent button when false) so tests that assert clearing-is-disabled reflect the real component behavior and the onClick handler is not exposed when allowClear is false.
🧹 Nitpick comments (5)
__tests__/hooks/useWaveConfig.test.ts (1)
27-42: Broaden this to the otheronGroupSelectbranches.The hook still has separate paths for
CAN_DROP,CAN_VOTE,CAN_CHAT, andADMIN, plus the clear-to-nullcase. Exercising onlyCAN_VIEWleaves most of that switch unguarded after the refactor. A small table-driven test would cover the rest cheaply.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/hooks/useWaveConfig.test.ts` around lines 27 - 42, Add table-driven cases to the existing test for useWaveConfig to exercise all onGroupSelect branches: call result.current.onGroupSelect with a sample group and each CreateWaveGroupConfigType value (CAN_VIEW, CAN_DROP, CAN_VOTE, CAN_CHAT, ADMIN) and assert that the corresponding config.groups.<field> (e.g., config.groups.canView, canDrop, canVote, canChat, admin) equals the group's id and that groupsCache["group-1"] equals the group; also include the clear-to-null case by calling onGroupSelect with group: null for one of the types and assert the corresponding config.groups.<field> becomes null. Use the existing group object and the onGroupSelect function name to locate where to add the looped assertions.__tests__/components/waves/create-wave/groups/CreateWaveGroupSearchField.test.tsx (1)
41-48: useDebounce mock has an eslint-exhaustive-deps-like issue.The mock calls
callbackinside auseEffectbut thecallbackitself isn't in the dependency array - onlydepsis passed. While this works for testing (it just runs synchronously), the pattern could cause stale closure issues if callback changes. Consider including callback in deps:useDebounce: ( callback: () => void, _delay: number, deps: DependencyList ) => { - React.useEffect(callback, deps); + React.useEffect(callback, [...deps, callback]); },However, since this is a test mock and the intent is to run synchronously, this is a minor concern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/waves/create-wave/groups/CreateWaveGroupSearchField.test.tsx` around lines 41 - 48, The useDebounce mock in the test calls React.useEffect(callback, deps) which omits the callback from the dependency array and can cause stale closure issues; update the mock for useDebounce (the function named useDebounce in this test) to include the callback in the dependency array (e.g., React.useEffect(callback, [callback, ...deps]) or combine callback with deps) so the effect re-runs when the callback changes while preserving the synchronous test behavior.__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx (1)
396-420: Mock placement after describe block is unconventional.The
@headlessui/reactmock is placed after thedescribeblock ends. While Jest hoistsjest.mock()calls to the top of the file regardless of placement, this makes the test file harder to read and maintain. Consider moving this mock to join the other mocks at the top of the file (around lines 12-185).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx` around lines 396 - 420, The jest.mock for "@headlessui/react" is placed after the describe block; move that jest.mock(...) declaration up with the other top-of-file mocks so it sits alongside the other mock setups (i.e., before any describe/it blocks) to improve readability and maintenance; keep the mock implementation exactly as-is (Menu, MenuButton, MenuItems, MenuItem, Transition and the local close jest.fn()) and ensure no duplicate identifiers collide when moved.components/waves/create-wave/groups/useCreateWaveGroupSearch.ts (1)
327-338: Complex conditional logic for clearing selection on input change.The logic for deciding whether to clear the selection when the user types is a bit complex: it clears only if there's a
selectedGroupANDallowClearis true. Consider adding a brief comment explaining this behavior for future maintainers.const handleInputChange = (value: string) => { + // Clear selection when user modifies input text (if clearing is allowed) setSearchState({ selectedGroupKey: selectedGroup && allowClear ? null : selectedGroupKey, inputValue: value, searchCriteria: value, activeIndex: -1, }); setIsOpen(true); if (selectedGroup && allowClear) { onSelect(null); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/waves/create-wave/groups/useCreateWaveGroupSearch.ts` around lines 327 - 338, The handleInputChange function currently clears the selected group only when both selectedGroup and allowClear are true, which is subtle; add a concise inline comment above the conditional (referencing handleInputChange, selectedGroup, allowClear, and onSelect) explaining that typing will reset the selection only when a selection exists and clearing is permitted, and note that onSelect(null) is invoked to notify consumers of the clear action.components/waves/specs/groups/group/edit/WaveGroupChangeDialog.tsx (1)
116-123: Using<dialog>withopenattribute but without native dialog methods.The
<dialog>element is rendered with theopenattribute, bypassing the nativeshowModal()method. This means the browser's built-in modal behavior (like automatic focus trapping and backdrop) isn't used. Since you're usingFocusTrapand a manual backdrop, this works, but consider whether a plain<div role="dialog">would be more semantically appropriate given the non-native usage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/waves/specs/groups/group/edit/WaveGroupChangeDialog.tsx` around lines 116 - 123, The dialog is rendered with a raw <dialog> element but not using native showModal()/close(), so either use the native API or switch to a semantic non-native element: update WaveGroupChangeDialog to either call modalRef.current.showModal() / close() (and manage backdrop/focus via the existing modalRef and lifecycle hooks) so the <dialog> uses native modal behavior, or replace the <dialog> tag with a <div role="dialog"> (preserving aria-modal="true", aria-labelledby={titleId}, aria-describedby={descriptionId}, tabIndex={-1} and the existing className and modalRef) and continue using FocusTrap and the manual backdrop; reference modalRef, titleId, descriptionId, and the component WaveGroupChangeDialog when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx`:
- Around line 26-30: The mock for useGroupMutations is incomplete—only submit
(mockSubmitInlineGroup) is provided while the real hook also exposes validate,
runTest, updateVisibility, isSubmitting, isTesting, and isUpdatingVisibility;
update the jest.mock so useGroupMutations returns stubbed implementations for
validate, runTest, updateVisibility and the boolean flags (isSubmitting,
isTesting, isUpdatingVisibility) alongside submit (use existing
mockSubmitInlineGroup) to mirror the real hook's shape and avoid undefined
property access in components or tests.
In `@components/waves/create-wave/groups/CreateWaveGroupSearchInput.tsx`:
- Around line 79-84: The aria-label on the clear button is generic and can be
misleading because showClearButton is true for both a typed query and a selected
group; update the button rendering (the element using showClearButton,
onClearSelection, and aria-label) to set aria-label dynamically based on
hasSelectedGroup (use "Clear selected group" when hasSelectedGroup is true,
otherwise "Clear search") so screen readers announce the correct action.
In `@components/waves/create-wave/groups/CreateWaveGroupSearchResults.tsx`:
- Around line 160-164: The loading branch in CreateWaveGroupSearchResults
renders only a spinner (CircleLoader with CircleLoaderSize.SMALL) which is
inaccessible to screen readers; wrap the spinner in a container with
role="status" and include a short visually-hidden label like "Loading groups"
(using the project's sr-only utility class, e.g., "sr-only" or "tw-sr-only") so
assistive tech announces the loading state when isFetching is true.
In `@components/waves/create-wave/groups/useCreateWaveGroupSearch.ts`:
- Around line 305-317: Escape presses currently trigger closeSearch() twice
because the input's onKeyDown handler (the input's Escape branch at the input
onKeyDown around lines 165-167) calls preventDefault() but not
stopPropagation(), and useKeyPressEvent("Escape") in useCreateWaveGroupSearch.ts
also listens globally; update the input's onKeyDown Escape branch to also call
event.stopPropagation() so the global useKeyPressEvent("Escape") does not
receive the event, or alternatively remove the useKeyPressEvent("Escape")
listener and rely solely on the input's onKeyDown; pick one approach, and ensure
references to the input onKeyDown handler and useKeyPressEvent("Escape") are
updated accordingly.
In `@components/waves/specs/groups/group/edit/WaveGroupChangeDialog.tsx`:
- Line 39: The assignment for created_by in WaveGroupChangeDialog.tsx uses
group.author (ApiGroup.author) which can be undefined while
ApiGroupFull.created_by expects ApiProfileMin; update the creation logic for
ApiGroupFull (the object assembled in the component) to handle the undefined
case by providing a safe fallback (e.g., map to a minimal ApiProfileMin
placeholder, null-check and omit created_by, or derive from current user
context) before assigning to created_by; locate the code where created_by:
group.author is set and replace it with a guarded assignment that ensures
created_by always matches ApiProfileMin or is handled explicitly by the API
call.
- Around line 30-44: getSelectedGroup currently returns an object cast to
ApiGroupFull but omits the required "group" field (ApiGroupDescription); fix by
populating the "group" property with a default ApiGroupDescription built from
available values (e.g., use group.name and set other optional fields to sensible
defaults such as empty description/avatar/metadata or nulls) so the returned
object conforms to ApiGroupFull, or alternatively change the function
signature/return type to reflect a partial (e.g., Partial<ApiGroupFull> | null)
if you intentionally want to omit that nested field; update getSelectedGroup to
implement one of these two approaches and keep the rest of the mapped fields
as-is.
---
Outside diff comments:
In
`@__tests__/components/waves/create-wave/groups/CreateWaveGroupInlinePanel.test.tsx`:
- Around line 45-69: The mock for CreateWaveGroupSearchField always renders an
active "clear group" button which ignores the allowClear prop; update the
MockCreateWaveGroupSearchField implementation to read props.allowClear and only
render the clear-control when allowClear is true (or render a disabled/absent
button when false) so tests that assert clearing-is-disabled reflect the real
component behavior and the onClick handler is not exposed when allowClear is
false.
---
Nitpick comments:
In
`@__tests__/components/waves/create-wave/groups/CreateWaveGroupSearchField.test.tsx`:
- Around line 41-48: The useDebounce mock in the test calls
React.useEffect(callback, deps) which omits the callback from the dependency
array and can cause stale closure issues; update the mock for useDebounce (the
function named useDebounce in this test) to include the callback in the
dependency array (e.g., React.useEffect(callback, [callback, ...deps]) or
combine callback with deps) so the effect re-runs when the callback changes
while preserving the synchronous test behavior.
In
`@__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx`:
- Around line 396-420: The jest.mock for "@headlessui/react" is placed after the
describe block; move that jest.mock(...) declaration up with the other
top-of-file mocks so it sits alongside the other mock setups (i.e., before any
describe/it blocks) to improve readability and maintenance; keep the mock
implementation exactly as-is (Menu, MenuButton, MenuItems, MenuItem, Transition
and the local close jest.fn()) and ensure no duplicate identifiers collide when
moved.
In `@__tests__/hooks/useWaveConfig.test.ts`:
- Around line 27-42: Add table-driven cases to the existing test for
useWaveConfig to exercise all onGroupSelect branches: call
result.current.onGroupSelect with a sample group and each
CreateWaveGroupConfigType value (CAN_VIEW, CAN_DROP, CAN_VOTE, CAN_CHAT, ADMIN)
and assert that the corresponding config.groups.<field> (e.g.,
config.groups.canView, canDrop, canVote, canChat, admin) equals the group's id
and that groupsCache["group-1"] equals the group; also include the clear-to-null
case by calling onGroupSelect with group: null for one of the types and assert
the corresponding config.groups.<field> becomes null. Use the existing group
object and the onGroupSelect function name to locate where to add the looped
assertions.
In `@components/waves/create-wave/groups/useCreateWaveGroupSearch.ts`:
- Around line 327-338: The handleInputChange function currently clears the
selected group only when both selectedGroup and allowClear are true, which is
subtle; add a concise inline comment above the conditional (referencing
handleInputChange, selectedGroup, allowClear, and onSelect) explaining that
typing will reset the selection only when a selection exists and clearing is
permitted, and note that onSelect(null) is invoked to notify consumers of the
clear action.
In `@components/waves/specs/groups/group/edit/WaveGroupChangeDialog.tsx`:
- Around line 116-123: The dialog is rendered with a raw <dialog> element but
not using native showModal()/close(), so either use the native API or switch to
a semantic non-native element: update WaveGroupChangeDialog to either call
modalRef.current.showModal() / close() (and manage backdrop/focus via the
existing modalRef and lifecycle hooks) so the <dialog> uses native modal
behavior, or replace the <dialog> tag with a <div role="dialog"> (preserving
aria-modal="true", aria-labelledby={titleId}, aria-describedby={descriptionId},
tabIndex={-1} and the existing className and modalRef) and continue using
FocusTrap and the manual backdrop; reference modalRef, titleId, descriptionId,
and the component WaveGroupChangeDialog when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28f79a33-1670-4f56-a146-91239df590a1
📒 Files selected for processing (20)
__tests__/components/waves/create-wave/CreateWave.test.tsx__tests__/components/waves/create-wave/groups/CreateWaveGroup.test.tsx__tests__/components/waves/create-wave/groups/CreateWaveGroupInlinePanel.test.tsx__tests__/components/waves/create-wave/groups/CreateWaveGroupSearchField.test.tsx__tests__/components/waves/create-wave/groups/CreateWaveGroups.test.tsx__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx__tests__/hooks/useWaveConfig.test.tscomponents/waves/create-wave/CreateWave.tsxcomponents/waves/create-wave/groups/CreateWaveGroup.tsxcomponents/waves/create-wave/groups/CreateWaveGroupInlinePanel.tsxcomponents/waves/create-wave/groups/CreateWaveGroupSearchField.tsxcomponents/waves/create-wave/groups/CreateWaveGroupSearchInput.tsxcomponents/waves/create-wave/groups/CreateWaveGroupSearchResults.tsxcomponents/waves/create-wave/groups/CreateWaveGroups.tsxcomponents/waves/create-wave/groups/CreateWaveInlineGroupSearch.tsxcomponents/waves/create-wave/groups/createWaveInlineGroupBuilder.tscomponents/waves/create-wave/groups/useCreateWaveGroupSearch.tscomponents/waves/create-wave/hooks/useWaveConfig.tscomponents/waves/specs/groups/group/edit/WaveGroupChangeDialog.tsxcomponents/waves/specs/groups/group/edit/WaveGroupEditButtons.tsx
💤 Files with no reviewable changes (5)
- tests/components/waves/create-wave/CreateWave.test.tsx
- components/waves/create-wave/CreateWave.tsx
- tests/components/waves/create-wave/groups/CreateWaveGroups.test.tsx
- components/waves/create-wave/hooks/useWaveConfig.ts
- components/waves/create-wave/groups/createWaveInlineGroupBuilder.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx (1)
111-114: AlignrenderTriggermock invocation with the real render-prop contract.Line 113 and Line 149 treat
renderTriggeras a component. The real prop shape is a callback (renderTrigger({ open })). Matching that shape improves test fidelity againstWaveGroupEditButton/WaveGroupRemoveButtonbehavior.Suggested fix
- if (renderTrigger) { - const Trigger = renderTrigger; - return <Trigger open={open} />; - } + if (renderTrigger) { + return renderTrigger({ open }); + }Apply the same change in both mock trigger helpers.
Also applies to: 147-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx` around lines 111 - 114, The tests treat renderTrigger like a React component but the real prop is a render-prop function; replace the JSX usage (const Trigger = renderTrigger; return <Trigger open={open} />) with a direct function call return renderTrigger({ open }), and make the same change in the other mock trigger helper used for WaveGroupEditButton/WaveGroupRemoveButton so both mocks match the renderTrigger({ open }) contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx`:
- Around line 387-391: The test currently asserts mutateAsync synchronously
after waiting for auth, which can be flaky; wrap the mutateAsync assertions in a
waitFor to make them async-safe: after awaiting waitFor(() =>
expect(auth.requestAuth).toHaveBeenCalled()), add a waitFor that checks
expect(mutateAsync).toHaveBeenCalled() and then assert the payload
(mutateAsync.mock.calls[0][0].visibility.scope.group_id === "group-2") inside
that waitFor so the test waits for mutateAsync to resolve before reading its
mock calls.
In `@components/waves/create-wave/groups/CreateWaveGroupSearchResults.tsx`:
- Around line 162-170: The CreateWaveGroupSearchResults component renders a
container with role="listbox" but its loading and empty rows are plain <li>
elements; update those rows (the loading block inside the isFetching branch and
the empty/helper branch) to include role="option" and appropriate ARIA state
(e.g., aria-disabled="true" for non-interactive rows) so every direct child of
the listbox meets ARIA listbox requirements; locate these blocks in
CreateWaveGroupSearchResults and add role="option" (and aria-disabled where
applicable) to the rendered element(s).
---
Nitpick comments:
In
`@__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsx`:
- Around line 111-114: The tests treat renderTrigger like a React component but
the real prop is a render-prop function; replace the JSX usage (const Trigger =
renderTrigger; return <Trigger open={open} />) with a direct function call
return renderTrigger({ open }), and make the same change in the other mock
trigger helper used for WaveGroupEditButton/WaveGroupRemoveButton so both mocks
match the renderTrigger({ open }) contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e873dea-8a7f-41d5-9098-3073acc9a266
📒 Files selected for processing (4)
__tests__/components/waves/specs/groups/group/edit/WaveGroupEditButtons.test.tsxcomponents/waves/create-wave/groups/CreateWaveGroupSearchInput.tsxcomponents/waves/create-wave/groups/CreateWaveGroupSearchResults.tsxcomponents/waves/specs/groups/group/edit/WaveGroupChangeDialog.tsx
✅ Files skipped from review due to trivial changes (1)
- components/waves/create-wave/groups/CreateWaveGroupSearchInput.tsx
ragnep
approved these changes
Apr 16, 2026
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements