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 55 minutes and 40 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 (7)
📝 WalkthroughWalkthroughAdds "@ALL" group-mention support across editor, creation, rendering, persistence, and wave notification preferences. Introduces a Lexical GroupMentionNode and transformer, threads Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Editor as Lexical Editor
participant Plugin as MentionsPlugin
participant Node as GroupMentionNode
participant SaveFlow as Drop Save Flow
participant Storage as API / Server
participant Renderer as Markdown Renderer
User->>Editor: Type "@all"
Editor->>Plugin: detect mention match
Plugin->>Plugin: check canMentionAll & normalized query == "all"
Plugin->>User: show "@all" option
User->>Plugin: select "@all"
Plugin->>Node: $createGroupMentionNode("@all")
Node->>Editor: replace text with GroupMentionNode
User->>SaveFlow: Save drop
SaveFlow->>Editor: read editor state
Editor->>SaveFlow: returns EditorState with GroupMentionNode
SaveFlow->>SaveFlow: getMentionedGroupsFromEditorState -> [ALL]
SaveFlow->>Storage: submit drop with mentioned_groups: [ALL]
User->>Renderer: View drop
Storage->>Renderer: drop + mentioned_groups
Renderer->>Renderer: tokenize content, mark `@all` tokens
Renderer->>User: render styled "@all"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/waves/CreateDropContent.tsx (1)
735-746:⚠️ Potential issue | 🟠 MajorDon't silently drop existing
@allmetadata when eligibility changes mid-compose.These aggregations all use the current
canMentionAllflag, so a draft that already containsmentioned_groups: [All]will be rewritten to[]ifauthenticated_user_eligible_for_adminflips false before submit. The edit flow already preserves existing group mentions viacanResolveAllGroupMention; create should preserve previously-added metadata too, or fail explicitly instead of mutating the draft.Also applies to: 779-786, 832-839, 916-921
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/waves/CreateDropContent.tsx` around lines 735 - 746, The current logic in getInitialDrop (and the other similar blocks at the indicated ranges) recomputes mentioned_groups using getMentionedGroupsFromParts(baseParts, canMentionAll), which will drop an existing `@all` mention if authenticated_user_eligible_for_admin (canMentionAll) flips false mid-compose; instead, preserve any already-present group metadata by preferring drop?.mentioned_groups when it exists (e.g., use drop?.mentioned_groups ?? getMentionedGroupsFromParts(baseParts, canMentionAll)), or explicitly detect and preserve the All group if present before falling back to recomputing—apply the same change to the other occurrences (around lines 779-786, 832-839, 916-921) and update any logic around canResolveAllGroupMention to ensure we fail explicitly only when creating/submitting, not while mutating the draft.components/waves/drops/EditDropLexical.tsx (1)
280-326:⚠️ Potential issue | 🟠 MajorAvoid reinitializing the editor when transformer props change.
This effect depends on
transformers, which is derived fromhasInitialAllGroupMention(itself computed frominitialGroupMentions). When permissions change at runtime (e.g.,canMentionAllbecomes true), theimportMarkdownTransformersarray updates, causing the effect to re-run and call$convertFromMarkdownString(...)with the initial content, overwriting any unsaved edits. Initialization should depend only on content changes, not on transformer configuration updates.Suggested fix
function InitialContentPlugin({ initialContent, transformers, }: { initialContent: string; transformers: Transformer[]; }) { const [editor] = useLexicalComposerContext(); + const lastLoadedContentRef = useRef<string | null>(null); useEffect(() => { + if (lastLoadedContentRef.current === initialContent) { + return; + } + lastLoadedContentRef.current = initialContent; + editor.update(() => { const normalizedContent = normalizeDropMarkdown(initialContent); $convertFromMarkdownString(normalizedContent, transformers); @@ root.selectEnd(); }); - }, [editor, initialContent, transformers]); + }, [editor, initialContent]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/waves/drops/EditDropLexical.tsx` around lines 280 - 326, The effect in InitialContentPlugin re-runs when the transformers prop changes and reinitializes the editor (calling $convertFromMarkdownString) which overwrites unsaved edits; change the effect to only run when initialContent (or editor) changes by removing transformers from the dependency array and capturing a stable transformers value for the initialization pass—e.g., compute a stableTransformers once (useMemo or useRef at mount using the current transformers) or read transformers into a local const inside the effect on first run—then call $convertFromMarkdownString(normalizedContent, stableTransformers) so runtime permission-based transformer updates no longer trigger editor reinitialization.
🧹 Nitpick comments (2)
components/drops/create/lexical/lexical.styles.scss (1)
23-25: Consider consolidating duplicate mention color selectors.
.editor-mention,.editor-wave-mention,.editor-group-mention, and.editor-hashtagall use the same color. Grouping them into one selector would reduce repetition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/drops/create/lexical/lexical.styles.scss` around lines 23 - 25, Several selectors (.editor-mention, .editor-wave-mention, .editor-group-mention, .editor-hashtag) repeat the same color; consolidate them into a single grouped selector to remove duplication by replacing the individual rules with one selector list (e.g., ".editor-mention, .editor-wave-mention, .editor-group-mention, .editor-hashtag") and apply the shared color there, leaving any unique styles separate.components/waves/drops/WaveDropPartDrop.tsx (1)
22-28: Verify everyonSavehandler against the new parameter order.Inserting
mentionedGroupsbeforementionedWavescreates a silently breaking positional API change. Any handler still written as(content, mentions, mentionedWaves)can keep compiling but will receive group mentions as its third argument at runtime. Consider switching this callback to a single object payload to make future additions safe.🔎 Safer callback shape
- readonly onSave?: - | (( - newContent: string, - mentions?: ApiDropMentionedUser[], - mentionedGroups?: ApiDropGroupMention[], - mentionedWaves?: ApiMentionedWave[] - ) => void) - | undefined; + readonly onSave?: + | ((payload: { + newContent: string; + mentions?: ApiDropMentionedUser[]; + mentionedGroups?: ApiDropGroupMention[]; + mentionedWaves?: ApiMentionedWave[]; + }) => void) + | undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/waves/drops/WaveDropPartDrop.tsx` around lines 22 - 28, The onSave callback signature in WaveDropPartDrop was changed to insert mentionedGroups before mentionedWaves, which is a breaking positional change; update every handler and call site that implements or invokes onSave (the onSave prop on WaveDropPartDrop and any functions passed into it) to match the new parameter order OR refactor the callback to a single object payload (e.g., onSave({ newContent, mentions, mentionedGroups, mentionedWaves })) and update the WaveDropPartDrop prop type and all callers/implementations accordingly so the runtime arguments are correct and future additions are non-breaking.
🤖 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/drops/view/part/DropPartMarkdown.test.tsx`:
- Around line 247-249: The test currently uses expect(screen.getByText("hello
`@all`")).not.toHaveClass("tw-text-primary-400"), which can miss a highlighted
`@all` inside a child element; change the assertion to explicitly check that no
element containing the token `@all` has the highlight class. For example, use
screen.queryAllByText(/@all/) (or screen.getByText to get the container) and
filter those elements by element.classList.contains("tw-text-primary-400") or
use container.querySelectorAll(".tw-text-primary-400") and ensure none of those
elements' textContent matches /@all/; update the assertion to
expect(theFilteredArray.length).toBe(0) so the test fails if any `@all` token
remains highlighted (references: screen.getByText, screen.queryAllByText,
toHaveClass).
In `@components/drops/create/lexical/nodes/GroupMentionNode.ts`:
- Around line 64-67: The override of createDOM in GroupMentionNode currently
replaces inherited theme/format classes by assigning dom.className; instead,
preserve Lexical's formatting classes added by super.createDOM(config) by
calling dom.classList.add("editor-group-mention") in the createDOM override so
setFormat() applied formats remain visible; update the createDOM method to use
classList.add rather than assigning className.
In `@components/waves/header/WaveHeaderFollow.tsx`:
- Line 158: The non-following icon's size mapping is being overridden by
hardcoded classes; update the className construction in WaveHeaderFollow so
`${SVG_CLASSES[size]}` controls icon dimensions instead of the fixed `tw-h-3
tw-w-3` — either remove those fixed classes or make them conditional (e.g.,
derive height/width from the `size` prop) where the class string currently
contains `${SVG_CLASSES[size]} -tw-ml-1 tw-h-3 tw-w-3 tw-flex-shrink-0`; ensure
`SVG_CLASSES`, the `size` prop, and the `className` on the non-following icon
are used consistently so the enum affects rendered size.
In
`@components/waves/specs/wave-notification-settings/useWavePreferenceSettings.ts`:
- Around line 29-40: The current all-drops UI state mixes the persisted
subscription flag with the subscriber-limit gate, so update the logic so
allDropsEnabled derives only from subscribedToAllDrops (keep const
allDropsEnabled = subscribedToAllDrops) and use disableAllDropsSelection
(computed from allDropsNotificationsSubscribersLimit and
wave.metrics.subscribers_count) solely to disable interaction in the UI
components; locate and change the assignment of allDropsEnabled and ensure any
UI binding that blocks clicks/inputs references disableAllDropsSelection instead
of toggling the enabled state.
In
`@components/waves/specs/wave-notification-settings/WaveMutedNotificationButton.tsx`:
- Around line 18-23: The Tooltip placement is set on the Tooltip component but
react-bootstrap expects the placement prop on OverlayTrigger; in
WaveMutedNotificationButton move the placement="top" prop from the Tooltip
element to the surrounding OverlayTrigger (the component rendering the overlay
for the Tooltip with id `mute-tooltip-${waveId}` and content
`settings.muteTooltip`) so the overlay positioning is honored by OverlayTrigger.
In
`@components/waves/specs/wave-notification-settings/WaveNotificationPreferenceButtons.tsx`:
- Around line 69-88: Wrap the disabled button element (the child of
OverlayTrigger) in a non-disabled wrapper so the overlay can receive hover/focus
events: replace the OverlayTrigger direct child button with a <span> wrapper
when settings.disableAllDropsSelection (and likewise for the all-group
button/disableAllGroupSelection) is true, e.g. have OverlayTrigger -> <span
className={settings.disableAllDropsSelection ? 'tw-inline-block' : ''}
style={settings.disableAllDropsSelection ? { cursor: 'not-allowed' } :
undefined}> -> <button disabled={...} ...>...</button> </span>; keep the button
disabled prop, keep the same aria-label, onClick handler and children
(Spinner/AllDropsIcon), and apply the same pattern to the other button so
Tooltip inside OverlayTrigger (Tooltip id={`all-drops-tooltip-${waveId}`}) will
appear even when the button is disabled.
In
`@components/waves/specs/wave-notification-settings/WaveNotificationRetryButton.tsx`:
- Around line 13-18: The Retry button in WaveNotificationRetryButton.tsx uses
the default button type (submit) which can accidentally submit parent forms;
update the JSX for the button element (the one using
settings.preferencesFetching and settings.onRetryClick) to include an explicit
type="button" attribute so it no longer triggers form submission when clicked.
In
`@components/waves/specs/wave-notification-settings/waveNotificationSettings.helpers.ts`:
- Around line 7-8: getErrorMessage currently only returns the error when it's a
string, dropping useful Error objects; update getErrorMessage to also handle
Error instances and objects with a message property by returning error.message
when error instanceof Error or when (error as any).message is a string, fallback
to the original string branch, and finally return defaultMessage if none match;
reference the getErrorMessage function to implement these checks.
In `@openapi.yaml`:
- Around line 11226-11234: The schema
ApiUpdateWaveNotificationPreferencesRequest currently allows subscribed: false
which conflicts with POST /notifications/wave-subscription/{wave_id}
(unsubscribe is handled via DELETE); update the OpenAPI schema so the subscribed
property cannot be false by making subscribed required and constrain it to true
(e.g., use const: true or enum: [true]) and keep enabled_group_notifications
as-is, ensuring generated clients cannot send subscribed: false for the POST
endpoint.
- Around line 7815-7818: The POST /drops/{dropId} update path currently uses the
ApiUpdateDropRequest schema which does not include the mentioned_groups field,
preventing edits to group mentions; update the OpenAPI contract by adding
mentioned_groups: array of ApiDropGroupMention to the ApiUpdateDropRequest
schema (matching the create/response schemas), and ensure the POST
/drops/{dropId} operation's requestBody references the updated
ApiUpdateDropRequest so clients can add/remove `@all` group mentions on edit.
---
Outside diff comments:
In `@components/waves/CreateDropContent.tsx`:
- Around line 735-746: The current logic in getInitialDrop (and the other
similar blocks at the indicated ranges) recomputes mentioned_groups using
getMentionedGroupsFromParts(baseParts, canMentionAll), which will drop an
existing `@all` mention if authenticated_user_eligible_for_admin (canMentionAll)
flips false mid-compose; instead, preserve any already-present group metadata by
preferring drop?.mentioned_groups when it exists (e.g., use
drop?.mentioned_groups ?? getMentionedGroupsFromParts(baseParts,
canMentionAll)), or explicitly detect and preserve the All group if present
before falling back to recomputing—apply the same change to the other
occurrences (around lines 779-786, 832-839, 916-921) and update any logic around
canResolveAllGroupMention to ensure we fail explicitly only when
creating/submitting, not while mutating the draft.
In `@components/waves/drops/EditDropLexical.tsx`:
- Around line 280-326: The effect in InitialContentPlugin re-runs when the
transformers prop changes and reinitializes the editor (calling
$convertFromMarkdownString) which overwrites unsaved edits; change the effect to
only run when initialContent (or editor) changes by removing transformers from
the dependency array and capturing a stable transformers value for the
initialization pass—e.g., compute a stableTransformers once (useMemo or useRef
at mount using the current transformers) or read transformers into a local const
inside the effect on first run—then call
$convertFromMarkdownString(normalizedContent, stableTransformers) so runtime
permission-based transformer updates no longer trigger editor reinitialization.
---
Nitpick comments:
In `@components/drops/create/lexical/lexical.styles.scss`:
- Around line 23-25: Several selectors (.editor-mention, .editor-wave-mention,
.editor-group-mention, .editor-hashtag) repeat the same color; consolidate them
into a single grouped selector to remove duplication by replacing the individual
rules with one selector list (e.g., ".editor-mention, .editor-wave-mention,
.editor-group-mention, .editor-hashtag") and apply the shared color there,
leaving any unique styles separate.
In `@components/waves/drops/WaveDropPartDrop.tsx`:
- Around line 22-28: The onSave callback signature in WaveDropPartDrop was
changed to insert mentionedGroups before mentionedWaves, which is a breaking
positional change; update every handler and call site that implements or invokes
onSave (the onSave prop on WaveDropPartDrop and any functions passed into it) to
match the new parameter order OR refactor the callback to a single object
payload (e.g., onSave({ newContent, mentions, mentionedGroups, mentionedWaves
})) and update the WaveDropPartDrop prop type and all callers/implementations
accordingly so the runtime arguments are correct and future additions are
non-breaking.
🪄 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: 636bca05-aed4-448b-8302-2a1ac10bc47b
⛔ Files ignored due to path filters (9)
generated/models/ApiCreateDropRequest.tsis excluded by!**/generated/**generated/models/ApiDrop.tsis excluded by!**/generated/**generated/models/ApiDropGroupMention.tsis excluded by!**/generated/**generated/models/ApiDropWithoutWave.tsis excluded by!**/generated/**generated/models/ApiLightDrop.tsis excluded by!**/generated/**generated/models/ApiUpdateDropRequest.tsis excluded by!**/generated/**generated/models/ApiUpdateWaveNotificationPreferencesRequest.tsis excluded by!**/generated/**generated/models/ApiWaveNotificationPreferences.tsis excluded by!**/generated/**generated/models/ObjectSerializer.tsis excluded by!**/generated/**
📒 Files selected for processing (62)
__tests__/components/drops/create/lexical/plugins/mentions/MentionsPlugin.test.tsx__tests__/components/drops/create/utils/storm/CreateDropStormViewPart.test.tsx__tests__/components/drops/view/item/content/DropListItemContentPart.test.tsx__tests__/components/drops/view/part/DropPartMarkdown.test.tsx__tests__/components/waves/CreateDropStormPart.test.tsx__tests__/components/waves/CreateDropStormParts.test.tsx__tests__/components/waves/drops/EditDropLexical.test.tsx__tests__/components/waves/drops/WaveDrop.test.tsx__tests__/components/waves/header/WaveHeader.test.tsx__tests__/components/waves/header/WaveHeaderFollow.test.tsx__tests__/components/waves/specs/WaveNotificationSettings.test.tsx__tests__/helpers/waves/drop-group-mentions.test.ts__tests__/hooks/useWaveNotificationSubscription.test.tscomponents/drops/create/lexical/lexical.styles.scsscomponents/drops/create/lexical/nodes/GroupMentionNode.tscomponents/drops/create/lexical/plugins/mentions/MentionsPlugin.tsxcomponents/drops/create/lexical/plugins/mentions/MentionsTypeaheadMenuItem.tsxcomponents/drops/create/lexical/transformers/GroupMentionTransformer.tscomponents/drops/create/lexical/utils/groupMentionDetection.tscomponents/drops/create/utils/storm/CreateDropStormView.tsxcomponents/drops/create/utils/storm/CreateDropStormViewPart.tsxcomponents/drops/create/utils/storm/CreateDropStormViewPartQuote.tsxcomponents/drops/view/item/content/DropListItemContentGroupMention.tsxcomponents/drops/view/item/content/DropListItemContentPart.tsxcomponents/drops/view/part/DropPart.tsxcomponents/drops/view/part/DropPartContent.tsxcomponents/drops/view/part/DropPartMarkdown.tsxcomponents/drops/view/part/DropPartMarkdownWithPropLogger.tsxcomponents/drops/view/part/dropPartMarkdown/content.tsxcomponents/waves/CreateDrop.tsxcomponents/waves/CreateDropContent.tsxcomponents/waves/CreateDropInput.tsxcomponents/waves/CreateDropStormPart.tsxcomponents/waves/CreateDropStormParts.tsxcomponents/waves/drops/EditDropLexical.tsxcomponents/waves/drops/WaveDrop.tsxcomponents/waves/drops/WaveDropContent.tsxcomponents/waves/drops/WaveDropPart.tsxcomponents/waves/drops/WaveDropPartContent.tsxcomponents/waves/drops/WaveDropPartContentMarkdown.tsxcomponents/waves/drops/WaveDropPartDrop.tsxcomponents/waves/drops/WaveDropQuote.tsxcomponents/waves/header/WaveHeader.tsxcomponents/waves/header/WaveHeaderFollow.tsxcomponents/waves/leaderboard/grid/WaveLeaderboardGridItem.tsxcomponents/waves/memes/submission/utils/buildPreviewDrop.tscomponents/waves/small-leaderboard/WaveSmallLeaderboardItemContent.tsxcomponents/waves/specs/WaveNotificationSettings.tsxcomponents/waves/specs/wave-notification-settings/WaveMutedNotificationButton.tsxcomponents/waves/specs/wave-notification-settings/WaveNotificationPreferenceButtons.tsxcomponents/waves/specs/wave-notification-settings/WaveNotificationRetryButton.tsxcomponents/waves/specs/wave-notification-settings/useWaveMuteSettings.tscomponents/waves/specs/wave-notification-settings/useWaveNotificationSettings.tscomponents/waves/specs/wave-notification-settings/useWavePreferenceSettings.tscomponents/waves/specs/wave-notification-settings/waveNotificationSettings.helpers.tscomponents/waves/utils/getOptimisticDrop.tsentities/IDrop.tshelpers/waves/drop-group-mentions.tshooks/drops/useDropUpdateMutation.tshooks/useIdentitiesSearch.tsxhooks/useWaveNotificationSubscription.tsopenapi.yaml
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
components/drops/create/lexical/nodes/GroupMentionNode.ts (1)
41-43:⚠️ Potential issue | 🟡 MinorKeep imported
groupMentionNameand rendered text in sync.Lines 41-43 currently allow
groupMentionNameandtextto diverge during import. That can cause downstream mismatch because detection relies ongetTextContent()(seecomponents/drops/create/lexical/utils/groupMentionDetection.ts, Line 21).Suggested fix
static override importJSON( serializedNode: SerializedGroupMentionNode ): GroupMentionNode { - const node = $createGroupMentionNode(serializedNode.groupMentionName); - node.setTextContent(serializedNode.text); + const normalizedMention = + serializedNode.groupMentionName || serializedNode.text; + const node = $createGroupMentionNode(normalizedMention); + node.setTextContent(normalizedMention); node.setFormat(serializedNode.format); node.setDetail(serializedNode.detail); node.setMode(serializedNode.mode);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/drops/create/lexical/nodes/GroupMentionNode.ts` around lines 41 - 43, The import currently sets node text from serializedNode.text which can diverge from the imported groupMentionName and break detection that uses getTextContent(); update the deserialization in the $createGroupMentionNode path so the node's text content is derived from (or overwritten by) serializedNode.groupMentionName (not serializedNode.text), and ensure node.setFormat(serializedNode.format) and any groupMentionName property on the created node are kept in sync with that same value (refer to $createGroupMentionNode, node.setTextContent and groupMentionName used by groupMentionDetection.ts).
🧹 Nitpick comments (1)
components/waves/header/WaveHeaderFollow.tsx (1)
178-187: Expose toggle state to assistive tech.This button is a stateful toggle (join/leave). Adding
aria-pressedimproves accessibility semantics for screen readers.♿ Suggested patch
<button onClick={onFollow} disabled={mutating} type="button" + aria-pressed={following} + aria-label={following ? "Leave wave" : "Join wave"} className={`${BUTTON_CLASSES[size]} ${ following ? "tw-bg-iron-800 tw-text-iron-300 tw-ring-iron-700 hover:tw-bg-iron-700 hover:tw-ring-iron-700" : "tw-bg-primary-500 tw-text-white tw-ring-primary-500 hover:tw-bg-primary-600 hover:tw-ring-primary-600" } ${fullWidth ? "tw-h-10 tw-w-full tw-justify-center lg:tw-h-9" : ""} tw-flex tw-cursor-pointer tw-items-center tw-rounded-lg tw-border-0 tw-font-semibold tw-ring-1 tw-ring-inset tw-transition tw-duration-300 tw-ease-out`} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/waves/header/WaveHeaderFollow.tsx` around lines 178 - 187, The follow button in WaveHeaderFollow (rendered in the button using onFollow, following, mutating, BUTTON_CLASSES and fullWidth) is a stateful toggle but lacks aria-pressed for assistive tech; update the button element to include aria-pressed={Boolean(following)} so screen readers know its pressed (joined) state, and ensure the prop value derives from the existing following boolean (coerced if necessary) without changing event handlers or visual styles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/drops/create/lexical/nodes/GroupMentionNode.ts`:
- Around line 21-24: convertGroupMentionElement currently passes
domNode.textContent (string | null) into $createGroupMentionNode which expects a
non-null string; update convertGroupMentionElement to guard the nullable value
before calling $createGroupMentionNode (e.g., read const text =
domNode.textContent ?? '' or return a null/empty DOMConversionOutput if text is
null) so that $createGroupMentionNode always receives a non-null string; adjust
the returned DOMConversionOutput accordingly in convertGroupMentionElement.
In
`@components/waves/specs/wave-notification-settings/WaveNotificationPreferenceButtons.tsx`:
- Around line 16-20: The all-drops button styling currently forces a
"not-allowed" disabled state whenever disableAllDropsSelection is true, which
prevents users who are already subscribed (settings.allDropsEnabled === true)
from unsubscribing when a wave crosses the limit; update getAllDropsButtonStyle
to only add "tw-cursor-not-allowed" when disableAllDropsSelection is true AND
settings.allDropsEnabled is false (i.e., disable selection only for enabling,
not for disabling). Apply the same conditional change to the other affected
helpers/components referenced (the blocks at 48-56 and 96-105) so they also
allow the unsubscribe action when already subscribed.
---
Duplicate comments:
In `@components/drops/create/lexical/nodes/GroupMentionNode.ts`:
- Around line 41-43: The import currently sets node text from
serializedNode.text which can diverge from the imported groupMentionName and
break detection that uses getTextContent(); update the deserialization in the
$createGroupMentionNode path so the node's text content is derived from (or
overwritten by) serializedNode.groupMentionName (not serializedNode.text), and
ensure node.setFormat(serializedNode.format) and any groupMentionName property
on the created node are kept in sync with that same value (refer to
$createGroupMentionNode, node.setTextContent and groupMentionName used by
groupMentionDetection.ts).
---
Nitpick comments:
In `@components/waves/header/WaveHeaderFollow.tsx`:
- Around line 178-187: The follow button in WaveHeaderFollow (rendered in the
button using onFollow, following, mutating, BUTTON_CLASSES and fullWidth) is a
stateful toggle but lacks aria-pressed for assistive tech; update the button
element to include aria-pressed={Boolean(following)} so screen readers know its
pressed (joined) state, and ensure the prop value derives from the existing
following boolean (coerced if necessary) without changing event handlers or
visual styles.
🪄 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: b9d9b6a7-2fa9-4b5c-8cd5-7725a490a6ba
📒 Files selected for processing (8)
__tests__/components/waves/specs/WaveNotificationSettings.test.tsxcomponents/drops/create/lexical/nodes/GroupMentionNode.tscomponents/waves/header/WaveHeaderFollow.tsxcomponents/waves/specs/wave-notification-settings/WaveMutedNotificationButton.tsxcomponents/waves/specs/wave-notification-settings/WaveNotificationPreferenceButtons.tsxcomponents/waves/specs/wave-notification-settings/WaveNotificationRetryButton.tsxcomponents/waves/specs/wave-notification-settings/useWavePreferenceSettings.tscomponents/waves/specs/wave-notification-settings/waveNotificationSettings.helpers.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- components/waves/specs/wave-notification-settings/WaveMutedNotificationButton.tsx
- components/waves/specs/wave-notification-settings/waveNotificationSettings.helpers.ts
- tests/components/waves/specs/WaveNotificationSettings.test.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
components/waves/specs/wave-notification-settings/WaveNotificationPreferenceButtons.tsx (1)
49-54: Declare both controls as non-submit buttons.These are JS toggles, not submit actions. Plain
<button>defaults to submit when it's inside or associated with a<form>, so reusing this component in a form can trigger an unintended submit. Addtype="button"to both buttons. (developer.mozilla.org)Suggested fix
const allDropsButton = ( <button + type="button" disabled={settings.loading || allDropsSelectionDisabled} onClick={settings.onAllDropsNotificationsClick} className={`tw-flex tw-h-10 tw-w-full tw-items-center tw-justify-center tw-whitespace-nowrap tw-rounded-lg tw-border tw-border-solid tw-border-iron-700 tw-px-2.5 tw-py-2 tw-transition tw-duration-300 tw-ease-out lg:tw-h-9 ${getAllDropsButtonStyle(settings)}`} aria-label="Receive all drop notifications" style={allDropsSelectionDisabled ? { pointerEvents: "none" } : undefined} @@ > <button + type="button" disabled={settings.loading} onClick={settings.onAllGroupNotificationsClick} className={`tw-flex tw-h-10 tw-w-full tw-items-center tw-justify-center tw-whitespace-nowrap tw-rounded-lg tw-border tw-border-solid tw-border-iron-700 tw-px-2.5 tw-py-2 tw-transition tw-duration-300 tw-ease-out lg:tw-h-9 ${getButtonStyle(settings.allGroupNotificationsEnabled)}`} aria-label="Receive ALL mention notifications" >Also applies to: 73-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/waves/specs/wave-notification-settings/WaveNotificationPreferenceButtons.tsx` around lines 49 - 54, The two toggle buttons in WaveNotificationPreferenceButtons are plain <button> elements (the one using settings.onAllDropsNotificationsClick and the other using settings.onSpecificDropsNotificationsClick) and must be declared non-submit; add type="button" to both button elements (the one that uses getAllDropsButtonStyle and the one controlled by allDropsSelectionDisabled / getSpecificDropsButtonStyle) so they don't trigger form submission when this component is used inside a form.__tests__/components/waves/specs/WaveNotificationSettings.test.tsx (1)
32-35: This mock removes the behavior the new wrapper is meant to protect.
OverlayTriggeris reduced tochildrenhere, so hover/focus handling and tooltip targeting never execute in tests. That means the DOM-structure assertions can stay green even if the real tooltip stops opening. I'd keep the simple mock for most parent-level cases, but add one focused test forWaveNotificationPreferenceButtonswith the real trigger behavior or a higher-fidelity mock.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/waves/specs/WaveNotificationSettings.test.tsx` around lines 32 - 35, The global mock for react-bootstrap replaces OverlayTrigger with a passthrough so hover/focus and tooltip behavior never run; update tests to keep the simple mock for most suites but add a focused test that uses the real trigger behavior: in the WaveNotificationPreferenceButtons test, undo or bypass the mock (use jest.unmock('react-bootstrap') or import the actual module via jest.requireActual) before importing/rendering WaveNotificationPreferenceButtons, render the real OverlayTrigger/Tooltip, simulate hover/focus on the target and assert the tooltip appears; alternatively implement a higher-fidelity mock of OverlayTrigger that forwards mouseEnter/mouseLeave/focus/blur to children to mimic real behavior and use that only in the WaveNotificationPreferenceButtons-focused spec.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/waves/specs/wave-notification-settings/WaveNotificationPreferenceButtons.tsx`:
- Around line 94-100: When allDropsSelectionDisabled is true the non-focusable
<span> wrapper around allDropsButton prevents keyboard users from discovering
the tooltip explanation; update the conditional rendering in
WaveNotificationPreferenceButtons (the block using allDropsSelectionDisabled and
allDropsButton) to render an element that is keyboard-focusable and exposes the
explanation to assistive tech (for example replace the <span> with a focusable
element or add tabIndex={0} and provide an accessible name/description via
aria-describedby or aria-label, and include aria-disabled or role="button" to
indicate its disabled state) so screen reader and keyboard users can access the
disabled-state message.
---
Nitpick comments:
In `@__tests__/components/waves/specs/WaveNotificationSettings.test.tsx`:
- Around line 32-35: The global mock for react-bootstrap replaces OverlayTrigger
with a passthrough so hover/focus and tooltip behavior never run; update tests
to keep the simple mock for most suites but add a focused test that uses the
real trigger behavior: in the WaveNotificationPreferenceButtons test, undo or
bypass the mock (use jest.unmock('react-bootstrap') or import the actual module
via jest.requireActual) before importing/rendering
WaveNotificationPreferenceButtons, render the real OverlayTrigger/Tooltip,
simulate hover/focus on the target and assert the tooltip appears; alternatively
implement a higher-fidelity mock of OverlayTrigger that forwards
mouseEnter/mouseLeave/focus/blur to children to mimic real behavior and use that
only in the WaveNotificationPreferenceButtons-focused spec.
In
`@components/waves/specs/wave-notification-settings/WaveNotificationPreferenceButtons.tsx`:
- Around line 49-54: The two toggle buttons in WaveNotificationPreferenceButtons
are plain <button> elements (the one using settings.onAllDropsNotificationsClick
and the other using settings.onSpecificDropsNotificationsClick) and must be
declared non-submit; add type="button" to both button elements (the one that
uses getAllDropsButtonStyle and the one controlled by allDropsSelectionDisabled
/ getSpecificDropsButtonStyle) so they don't trigger form submission when this
component is used inside a form.
🪄 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: 845d7577-6ca2-4bd5-a924-9a374a237477
📒 Files selected for processing (4)
__tests__/components/waves/specs/WaveNotificationSettings.test.tsxcomponents/drops/create/lexical/nodes/GroupMentionNode.tscomponents/waves/specs/wave-notification-settings/WaveNotificationPreferenceButtons.tsxcomponents/waves/specs/wave-notification-settings/waveNotificationSettings.helpers.ts
✅ Files skipped from review due to trivial changes (1)
- components/waves/specs/wave-notification-settings/waveNotificationSettings.helpers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- components/drops/create/lexical/nodes/GroupMentionNode.ts
|
ragnep
approved these changes
Apr 15, 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
Style / UI