Merged
Conversation
📝 WalkthroughWalkthroughAdds an overlay-based actions UI for link previews (ChatItemHrefButtons) with keyboard and focus management, viewport-aware dropdown positioning and focus-outside closing, a LinkPreviewContext callback for card-action activity, and propagation of link-card action state to suppress WaveDrop actions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LinkCard as ChatItemHrefButtons
participant Dropdown as CommonDropdownItemsDefaultWrapper
participant WaveDrop as WaveDrop
User->>LinkCard: Activate trigger (click/Enter)
LinkCard->>Dropdown: open()
Dropdown->>Dropdown: calculate position (above/below, clamp)
Dropdown-->>User: render overlay menu (focus moves to first action)
LinkCard->>WaveDrop: onLinkCardActionsActiveChange(href, true)
User->>Dropdown: Tab / activate action / Escape
Dropdown->>LinkCard: close() (restore focus as needed)
LinkCard->>WaveDrop: onLinkCardActionsActiveChange(href, false)
sequenceDiagram
participant LinkCard
participant WaveDrop
participant DropActions as WaveDropActions
LinkCard->>WaveDrop: onLinkCardActionsActiveChange(id, true)
WaveDrop->>WaveDrop: add id to active list → hasActiveLinkCardActions=true
WaveDrop->>DropActions: render with suppressed=true
DropActions->>DropActions: apply visibilityClasses (hidden/non-interactive)
LinkCard->>WaveDrop: onLinkCardActionsActiveChange(id, false)
WaveDrop->>WaveDrop: remove id → hasActiveLinkCardActions=false
WaveDrop->>DropActions: render with suppressed=false
DropActions->>DropActions: restore visibility (hover/tab behavior)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
📝 Coding Plan
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 Tip CodeRabbit can generate a title for your PR based on the changes.Add |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (7)
components/waves/drops/WaveDropPartDrop.tsx (1)
10-34: Optional: Inconsistentreadonlymodifiers in interface.Some props use the
readonlymodifier (e.g.,onSave,onCancel,onLinkCardActionsActiveChange) while others don't (e.g.,drop,activePart,onQuoteClick). Consider applyingreadonlyconsistently across all props for uniformity.🤖 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 10 - 34, The WaveDropPartDropProps interface mixes readonly and non-readonly property declarations, creating an inconsistent prop mutability contract; update WaveDropPartDropProps to use readonly consistently across all properties (e.g., drop, activePart, havePreviousPart, haveNextPart, isStorm, activePartIndex, setActivePartIndex, onQuoteClick, isEditing, isSaving, onSave, onCancel, isCompetitionDrop, mediaImageScale, onLinkCardActionsActiveChange) so every prop is either readonly or not—prefer adding readonly to every property to match the existing readonly ones and keep the interface uniform.__tests__/components/waves/drops/WaveDropQuote.test.tsx (1)
139-161: Consider adding a test for prop precedence over context.The current tests verify explicit prop and context fallback separately. Consider adding a test case where both are provided to verify that the explicit prop takes precedence over the context value (testing the
??operator behavior).📝 Example test case
test("explicit prop takes precedence over context", () => { const drop = { id: "d1", serial_no: 42, wave: { id: "w1", name: "wave" }, author: { handle: "a", level: 1, cic: "BRONZE", pfp: null }, parts: [{ part_id: 5, content: "text" }], created_at: "2020-01-01", mentioned_users: [], referenced_nfts: [], } as any; const explicitCallback = jest.fn(); const contextCallback = jest.fn(); render( <LinkPreviewProvider onCardActionsActiveChange={contextCallback}> <WaveDropQuote drop={drop} partId={5} onQuoteClick={jest.fn()} onLinkCardActionsActiveChange={explicitCallback} /> </LinkPreviewProvider> ); expect(markdownProps.onLinkCardActionsActiveChange).toBe(explicitCallback); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/waves/drops/WaveDropQuote.test.tsx` around lines 139 - 161, Add a test that ensures an explicit onLinkCardActionsActiveChange prop on the WaveDropQuote component overrides the LinkPreviewProvider context value: render WaveDropQuote inside LinkPreviewProvider with a context callback and pass a different explicit callback via the onLinkCardActionsActiveChange prop, then assert markdownProps.onLinkCardActionsActiveChange is the explicit callback; reference WaveDropQuote, LinkPreviewProvider, and markdownProps.onLinkCardActionsActiveChange when locating where to add the test in WaveDropQuote.test.tsx.components/waves/ChatItemHrefButtons.tsx (2)
258-272: Minor: Redundant optional chaining onelement.At line 263,
element?.hasAttribute("disabled")uses optional chaining, butelementis already confirmed to be truthy by theBoolean(element)check in the same predicate. This is harmless but slightly redundant.♻️ Simplify the predicate
const firstEnabledMenuItem = [ previewToggleButtonRef.current, copyButtonRef.current, openLinkButtonRef.current, ].find((element): element is HTMLButtonElement | HTMLAnchorElement => { - return Boolean(element) && !element?.hasAttribute("disabled"); + return element !== null && !element.hasAttribute("disabled"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/waves/ChatItemHrefButtons.tsx` around lines 258 - 272, The predicate passed to .find for computing firstEnabledMenuItem redundantly uses optional chaining on element (element?.hasAttribute("disabled")) even though Boolean(element) already ensures element is truthy; update the predicate in the block that builds firstEnabledMenuItem (which iterates previewToggleButtonRef.current, copyButtonRef.current, openLinkButtonRef.current) to call element.hasAttribute("disabled") without the unnecessary ?. so the check becomes Boolean(element) && !element.hasAttribute("disabled").
183-196: Consider usingrequestAnimationFrameorqueueMicrotaskinstead ofsetTimeout(0)for focus restoration.The
setTimeout(0)at line 188 is used to defer focus restoration, but this can be unreliable across different browsers and can be affected by minimum timer clamping (4ms in some cases). For more predictable focus management timing, consider:♻️ Alternative timing approaches
useEffect(() => { if (isMenuOpen || !shouldRestoreTriggerFocusRef.current) { return; } - const timeoutId = window.setTimeout(() => { + // Use queueMicrotask for more immediate, predictable timing + let cancelled = false; + queueMicrotask(() => { + if (cancelled) return; shouldRestoreTriggerFocusRef.current = false; buttonRef.current?.focus(); - }, 0); + }); return () => { - window.clearTimeout(timeoutId); + cancelled = true; }; }, [isMenuOpen]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/waves/ChatItemHrefButtons.tsx` around lines 183 - 196, The useEffect that restores focus uses window.setTimeout(() => { shouldRestoreTriggerFocusRef.current = false; buttonRef.current?.focus(); }, 0) which can be unreliable; replace the setTimeout scheduling with a more predictable mechanism such as requestAnimationFrame (store the returned id and cancel it with cancelAnimationFrame in the cleanup) or queueMicrotask (invoke immediately without a cancellable id) to defer execution; update the effect to schedule focus restoration with requestAnimationFrame (or queueMicrotask if you prefer) and ensure the cleanup cancels the frame using cancelAnimationFrame, keeping the same checks for isMenuOpen and shouldRestoreTriggerFocusRef and still setting shouldRestoreTriggerFocusRef.current = false before calling buttonRef.current?.focus().components/waves/LinkHandlerFrame.tsx (1)
38-52: TheoverlayAnchor="content"layout is not applied whenhideActionsis true.When
hideActionsis true (e.g., whenvariant="home"perLinkPreviewContext.tsx:56),actionButtonsisnull, causingshouldAnchorOverlayToContentto always befalse. This means the "content" anchor wrapper structure (lines 42-51) is never used when actions are hidden, even if explicitly requested viaoverlayAnchor="content".If the wrapper structure is important for layout consistency regardless of whether actions are shown, this logic should be revised. If this is intentional behavior, consider documenting it.
♻️ Option to apply content anchor layout regardless of action visibility
const shouldAnchorOverlayToContent = - overlayAnchor === "content" && actionButtons !== null; + overlayAnchor === "content"; if (shouldAnchorOverlayToContent) { return ( <div className="tw-flex tw-w-full tw-min-w-0 tw-max-w-full"> <div className="tw-group/link-card tw-relative tw-inline-flex tw-w-fit tw-min-w-0 tw-max-w-full tw-flex-col"> <div className="tw-min-w-0 tw-max-w-full tw-overflow-hidden focus-within:tw-overflow-visible"> {children} </div> - {actionButtons} + {!hideActions && actionButtons} </div> </div> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/waves/LinkHandlerFrame.tsx` around lines 38 - 52, The current check uses shouldAnchorOverlayToContent = overlayAnchor === "content" && actionButtons !== null so when hideActions makes actionButtons null the "content" wrapper is skipped; change the condition to only test overlayAnchor (e.g., shouldAnchorOverlayToContent = overlayAnchor === "content") and keep rendering actionButtons conditionally inside the wrapper (leave the existing {actionButtons} slot), so the content-anchored wrapper is applied even when actionButtons is null; update any related comments and references (overlayAnchor, actionButtons, shouldAnchorOverlayToContent, and LinkPreviewContext usage) accordingly.__tests__/components/waves/drops/WaveDropActions.test.tsx (2)
60-64: Consider using a partial type instead ofanyfor better type safety.Using
anybypasses type checking. A partial mock type would catch property typos and ensure the fixture aligns with the actual API shape.✏️ Suggested improvement
-const baseDrop: any = { +const baseDrop = { id: "drop-1", wave: { id: "wave-1" }, drop_type: ApiDropType.Chat, -}; +} as Partial<ExtendedDrop> as ExtendedDrop;You'll need to import
ExtendedDropfrom@/helpers/waves/drop.helpers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/waves/drops/WaveDropActions.test.tsx` around lines 60 - 64, Replace the loose any-typed fixture with a partial of the real drop type: import ExtendedDrop from "@/helpers/waves/drop.helpers" and declare baseDrop as const baseDrop: Partial<ExtendedDrop> = { id: "drop-1", wave: { id: "wave-1" }, drop_type: ApiDropType.Chat }; this preserves the minimal fixture shape while enabling type checking (catching typos and mismatches) and still allows omitted properties in tests; update any tests that assumed any-specific behavior accordingly.
82-98: Consider clarifying the test name to reflect the scenario being tested.The test renders with
suppressed={true}but the test name doesn't mention this. The test is actually verifying that the dropdown open state takes priority over the suppressed state - a specific edge case worth highlighting.✏️ Suggested rename
- it("keeps actions interactive when the more dropdown is open", () => { + it("dropdown open state overrides suppressed state", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/waves/drops/WaveDropActions.test.tsx` around lines 82 - 98, Rename the test in WaveDropActions.test.tsx to clearly state that the dropdown being open overrides the suppressed prop (i.e., the component keeps actions interactive even when suppressed is true); update the it(...) description for the test that renders <WaveDropActions drop={baseDrop} activePartIndex={0} onReply={() => {}} suppressed={true} /> and asserts container.firstElementChild has classes "tw-pointer-events-auto" and "tw-opacity-100" so the name explicitly mentions the suppressed=true edge case and that the open-more-actions state takes priority.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@__tests__/components/waves/drops/WaveDropActions.test.tsx`:
- Around line 60-64: Replace the loose any-typed fixture with a partial of the
real drop type: import ExtendedDrop from "@/helpers/waves/drop.helpers" and
declare baseDrop as const baseDrop: Partial<ExtendedDrop> = { id: "drop-1",
wave: { id: "wave-1" }, drop_type: ApiDropType.Chat }; this preserves the
minimal fixture shape while enabling type checking (catching typos and
mismatches) and still allows omitted properties in tests; update any tests that
assumed any-specific behavior accordingly.
- Around line 82-98: Rename the test in WaveDropActions.test.tsx to clearly
state that the dropdown being open overrides the suppressed prop (i.e., the
component keeps actions interactive even when suppressed is true); update the
it(...) description for the test that renders <WaveDropActions drop={baseDrop}
activePartIndex={0} onReply={() => {}} suppressed={true} /> and asserts
container.firstElementChild has classes "tw-pointer-events-auto" and
"tw-opacity-100" so the name explicitly mentions the suppressed=true edge case
and that the open-more-actions state takes priority.
In `@__tests__/components/waves/drops/WaveDropQuote.test.tsx`:
- Around line 139-161: Add a test that ensures an explicit
onLinkCardActionsActiveChange prop on the WaveDropQuote component overrides the
LinkPreviewProvider context value: render WaveDropQuote inside
LinkPreviewProvider with a context callback and pass a different explicit
callback via the onLinkCardActionsActiveChange prop, then assert
markdownProps.onLinkCardActionsActiveChange is the explicit callback; reference
WaveDropQuote, LinkPreviewProvider, and
markdownProps.onLinkCardActionsActiveChange when locating where to add the test
in WaveDropQuote.test.tsx.
In `@components/waves/ChatItemHrefButtons.tsx`:
- Around line 258-272: The predicate passed to .find for computing
firstEnabledMenuItem redundantly uses optional chaining on element
(element?.hasAttribute("disabled")) even though Boolean(element) already ensures
element is truthy; update the predicate in the block that builds
firstEnabledMenuItem (which iterates previewToggleButtonRef.current,
copyButtonRef.current, openLinkButtonRef.current) to call
element.hasAttribute("disabled") without the unnecessary ?. so the check becomes
Boolean(element) && !element.hasAttribute("disabled").
- Around line 183-196: The useEffect that restores focus uses
window.setTimeout(() => { shouldRestoreTriggerFocusRef.current = false;
buttonRef.current?.focus(); }, 0) which can be unreliable; replace the
setTimeout scheduling with a more predictable mechanism such as
requestAnimationFrame (store the returned id and cancel it with
cancelAnimationFrame in the cleanup) or queueMicrotask (invoke immediately
without a cancellable id) to defer execution; update the effect to schedule
focus restoration with requestAnimationFrame (or queueMicrotask if you prefer)
and ensure the cleanup cancels the frame using cancelAnimationFrame, keeping the
same checks for isMenuOpen and shouldRestoreTriggerFocusRef and still setting
shouldRestoreTriggerFocusRef.current = false before calling
buttonRef.current?.focus().
In `@components/waves/drops/WaveDropPartDrop.tsx`:
- Around line 10-34: The WaveDropPartDropProps interface mixes readonly and
non-readonly property declarations, creating an inconsistent prop mutability
contract; update WaveDropPartDropProps to use readonly consistently across all
properties (e.g., drop, activePart, havePreviousPart, haveNextPart, isStorm,
activePartIndex, setActivePartIndex, onQuoteClick, isEditing, isSaving, onSave,
onCancel, isCompetitionDrop, mediaImageScale, onLinkCardActionsActiveChange) so
every prop is either readonly or not—prefer adding readonly to every property to
match the existing readonly ones and keep the interface uniform.
In `@components/waves/LinkHandlerFrame.tsx`:
- Around line 38-52: The current check uses shouldAnchorOverlayToContent =
overlayAnchor === "content" && actionButtons !== null so when hideActions makes
actionButtons null the "content" wrapper is skipped; change the condition to
only test overlayAnchor (e.g., shouldAnchorOverlayToContent = overlayAnchor ===
"content") and keep rendering actionButtons conditionally inside the wrapper
(leave the existing {actionButtons} slot), so the content-anchored wrapper is
applied even when actionButtons is null; update any related comments and
references (overlayAnchor, actionButtons, shouldAnchorOverlayToContent, and
LinkPreviewContext usage) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ebfb85e-c729-44bd-960e-b3b0c3d4c05c
📒 Files selected for processing (26)
__tests__/components/ChatItemHrefButtons.test.tsx__tests__/components/utils/select/dropdown/CommonDropdownItemsDefaultWrapper.test.tsx__tests__/components/waves/LinkHandlerFrame.test.tsx__tests__/components/waves/drops/WaveDrop.test.tsx__tests__/components/waves/drops/WaveDropActions.test.tsx__tests__/components/waves/drops/WaveDropPartContentMarkdown.test.tsx__tests__/components/waves/drops/WaveDropQuote.test.tsxcomponents/drops/view/part/DropPartMarkdown.tsxcomponents/drops/view/part/DropPartMarkdownWithPropLogger.tsxcomponents/drops/view/part/dropPartMarkdown/renderers.tsxcomponents/drops/view/part/dropPartMarkdown/youtubePreview.tsxcomponents/utils/select/dropdown/CommonDropdownItemsDefaultWrapper.tsxcomponents/waves/ChatItemHrefButtons.tsxcomponents/waves/LinkHandlerFrame.tsxcomponents/waves/LinkPreviewContext.tsxcomponents/waves/OpenGraphPreview.tsxcomponents/waves/drops/WaveDrop.tsxcomponents/waves/drops/WaveDropActions.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/drops/WaveDropQuoteWithDropId.tsxcomponents/waves/drops/WaveDropQuoteWithSerialNo.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (2)
components/waves/ChatItemHrefButtons.tsx (2)
135-145: Consider extracting overlay-specific logic to reduce cognitive complexity.SonarCloud flags this function's cognitive complexity (21) exceeds the threshold (15). The component handles two distinct layout modes with different rendering paths and behaviors.
Consider extracting the overlay-specific logic into a separate component (e.g.,
ChatItemHrefButtonsOverlay) or extracting focus management into a custom hook. This would improve maintainability and testability while reducing the cognitive load of the main component.♻️ Suggested approach
// Option 1: Extract overlay variant function ChatItemHrefButtonsOverlay({ href, relativeHref, hideLink, previewToggle, onCardActionsActiveChange }) { // Move all overlay-specific state, effects, and rendering here } // Option 2: Extract focus management hook function useOverlayFocusManagement(buttonRef, isMenuOpen) { // Move focus restoration, escape handling, and related refs here return { shouldFocusFirstMenuItemRef, closeMenu, ... }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/waves/ChatItemHrefButtons.tsx` around lines 135 - 145, The ChatItemHrefButtons component has overlay-specific state and focus/escape handling that increases cognitive complexity; extract that logic into a separate component or hook: create a new ChatItemHrefButtonsOverlay component to move all overlay-only state/effects/rendering (including previewToggle, menu open/close handlers, and callbacks such as onCardActionsActiveChange) and use the existing ChatItemHrefButtons only to choose between rail vs overlay render; alternatively implement a useOverlayFocusManagement hook that accepts refs like buttonRef and shouldFocusFirstMenuItemRef and returns helpers (closeMenu, restoreFocus, escape handler) so the main ChatItemHrefButtons function only wires props and delegates overlay behavior to the new component/hook.
426-447: Addaria-labelto the open link element for consistent accessibility.The copy button and preview toggle button have
aria-labelattributes, but the open link anchor (openLinkButton) relies on the visible "Open link" text only in overlay mode. For consistency and to ensure screen readers announce the purpose in both modes, consider adding an explicitaria-label.♻️ Suggested change
const openLinkButton = hideLink ? null : ( <Link ref={handleOpenLinkButtonRef} href={relativeHref ?? href} target={relativeHref ? undefined : "_blank"} className={isOverlay ? MENU_ITEM_CLASSES : RAIL_BUTTON_CLASSES} + aria-label="Open link" onClick={(event) => { stopPropagation(event); closeMenu({ restoreFocusToTrigger: event.detail === 0 }); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/waves/ChatItemHrefButtons.tsx` around lines 426 - 447, The open link anchor (openLinkButton) lacks an explicit aria-label which makes screen-reader behavior inconsistent with the other buttons; add an aria-label prop to the Link component (the same element using handleOpenLinkButtonRef, href/relativeHref, isOverlay) that describes the action (e.g., "Open link") so the purpose is announced in both overlay and rail modes — ensure the aria-label string is present regardless of isOverlay and remains descriptive for relativeHref vs href cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@components/waves/ChatItemHrefButtons.tsx`:
- Around line 135-145: The ChatItemHrefButtons component has overlay-specific
state and focus/escape handling that increases cognitive complexity; extract
that logic into a separate component or hook: create a new
ChatItemHrefButtonsOverlay component to move all overlay-only
state/effects/rendering (including previewToggle, menu open/close handlers, and
callbacks such as onCardActionsActiveChange) and use the existing
ChatItemHrefButtons only to choose between rail vs overlay render; alternatively
implement a useOverlayFocusManagement hook that accepts refs like buttonRef and
shouldFocusFirstMenuItemRef and returns helpers (closeMenu, restoreFocus, escape
handler) so the main ChatItemHrefButtons function only wires props and delegates
overlay behavior to the new component/hook.
- Around line 426-447: The open link anchor (openLinkButton) lacks an explicit
aria-label which makes screen-reader behavior inconsistent with the other
buttons; add an aria-label prop to the Link component (the same element using
handleOpenLinkButtonRef, href/relativeHref, isOverlay) that describes the action
(e.g., "Open link") so the purpose is announced in both overlay and rail modes —
ensure the aria-label string is present regardless of isOverlay and remains
descriptive for relativeHref vs href cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3aa14f97-4f3e-43e5-bff7-26648f52d70e
📒 Files selected for processing (3)
__tests__/components/ChatItemHrefButtons.test.tsxcomponents/waves/ChatItemHrefButtons.tsxcomponents/waves/LinkHandlerFrame.tsx
|
ragnep
approved these changes
Mar 17, 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
UI/UX Improvements
Tests