feat/improve show more btn#155
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughRefactors CollapsibleMessage to forwardRef with an imperative toggle, adds hoverOnly and onNeedsCollapsing props, replaces animation state with measurement/layout effects, anchors scroll during expansion, exposes tap-to-toggle behavior via SwipeableMessage for touch devices, updates CSS hover reveal rules, and adds tests. Changes
Sequence DiagramsequenceDiagram
participant User
participant SwipeableMessage
participant CollapsibleMessage
participant ResizeObserver
User->>SwipeableMessage: touchstart (record target & pos)
User->>SwipeableMessage: touchmove (detect movement)
alt minimal movement & not long-press
User->>SwipeableMessage: touchend
SwipeableMessage->>SwipeableMessage: validate tap (not in button)
SwipeableMessage->>CollapsibleMessage: onTap -> toggle() via ref
CollapsibleMessage->>CollapsibleMessage: measure content, set collapsedMaxHeight
CollapsibleMessage->>ResizeObserver: attach/respect observer if anchored
ResizeObserver->>CollapsibleMessage: notify size changes to maintain anchor
CollapsibleMessage->>User: animate expand/collapse (max-height change)
else movement -> swipe/long-press
SwipeableMessage->>User: handle swipe/long-press flow
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
Automated deployment preview for the PR in the Cloudflare Pages. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/components/message/CollapsibleMessage.tsx (1)
79-79:useImperativeHandlemissing dependency array may cause stale closure.Without a dependency array, the handle is recreated on every render, which is wasteful. However, adding
[toggleExpanded]would also recreate frequently sincetoggleExpandedisn't memoized. Consider wrapping withuseCallbackor using a ref-based approach.🔧 Option 1: Add dependency array with memoized toggle
+ const toggleExpanded = useCallback(() => { + setIsExpanded((prev) => { + const willExpand = !prev; + if (willExpand && contentRef.current) { + // ... scroll anchoring logic + } + return willExpand; + }); + }, []); - useImperativeHandle(ref, () => ({ toggle: toggleExpanded })); + useImperativeHandle(ref, () => ({ toggle: toggleExpanded }), [toggleExpanded]);🔧 Option 2: Use functional update to avoid stale state
- useImperativeHandle(ref, () => ({ toggle: toggleExpanded })); + useImperativeHandle(ref, () => ({ + toggle: () => setIsExpanded((prev) => !prev), + }), []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/message/CollapsibleMessage.tsx` at line 79, The useImperativeHandle call currently has no dependency array which recreates the handle every render and risks stale closure; update the code so the exposed toggle is stable by either memoizing the toggleExpanded function with useCallback (e.g., wrap toggleExpanded in useCallback and then call useImperativeHandle(ref, () => ({ toggle: toggleExpanded }), [toggleExpanded])) or implement toggleExpanded as a stable function using a ref + functional state update and pass an empty or appropriate dependency array to useImperativeHandle; reference the useImperativeHandle and toggleExpanded symbols when making the change.tests/components/CollapsibleMessage.test.tsx (1)
17-28: Consider restoringscrollHeightproperty inafterEachto prevent test pollution.The
Object.definePropertyonHTMLElement.prototype.scrollHeightpersists after the test completes because onlymockScrollHeightis reset to 0, but the property descriptor remains. Whileconfigurable: trueallows re-definition, it's safer to explicitly delete or restore the original descriptor.🔧 Suggested cleanup
+ let originalScrollHeightDescriptor: PropertyDescriptor | undefined; + beforeEach(() => { vi.spyOn(window, "getComputedStyle").mockImplementation( () => ({ lineHeight: `${LINE_HEIGHT}px`, }) as unknown as CSSStyleDeclaration, ); + originalScrollHeightDescriptor = Object.getOwnPropertyDescriptor( + HTMLElement.prototype, + "scrollHeight", + ); Object.defineProperty(HTMLElement.prototype, "scrollHeight", { configurable: true, get() { return mockScrollHeight; }, }); }); afterEach(() => { vi.restoreAllMocks(); mockScrollHeight = 0; + if (originalScrollHeightDescriptor) { + Object.defineProperty( + HTMLElement.prototype, + "scrollHeight", + originalScrollHeightDescriptor, + ); + } else { + delete (HTMLElement.prototype as any).scrollHeight; + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/components/CollapsibleMessage.test.tsx` around lines 17 - 28, The test sets a custom descriptor on HTMLElement.prototype.scrollHeight but only resets mockScrollHeight in afterEach, leaving the descriptor defined and causing test pollution; capture the original descriptor (e.g., const originalScrollHeightDesc = Object.getOwnPropertyDescriptor(HTMLElement.prototype, "scrollHeight")) before calling Object.defineProperty in the setup and then restore it in afterEach (either by re-defining the original descriptor if it exists or deleting the property to fall back to the prototype chain) so that tests clean up the prototype change; reference the existing mockScrollHeight and the prototype modification to find where to save/restore the descriptor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/message/CollapsibleMessage.tsx`:
- Around line 33-48: The useLayoutEffect in CollapsibleMessage currently only
depends on maxLines and onNeedsCollapsing so it won't re-measure when the
rendered message content changes; update the dependency array for the effect
(the useLayoutEffect that reads contentRef, computes lineHeight, fullHeight,
calls setContentHeight, setCollapsedMaxHeight, setNeedsCollapsing and
onNeedsCollapsing) to also include a signal for content changes (e.g., the
content prop or a stable contentHash/key) so the effect reruns and recalculates
collapsing whenever the message content updates.
In `@src/components/message/SwipeableMessage.tsx`:
- Around line 145-150: The tap handler currently blocks only native buttons
(closest("button")) which lets clicks on interactive non-button elements (e.g.,
the clickable divs in MessageReactions) also trigger onTap; update the guard in
the onTap logic (where wasLongPress, hasMovedRef.current, onTap and
touchStartTargetRef.current are used) to check for a wider set of interactive
targets (e.g., button, [role="button"], a[href], input, textarea, select, and
elements with a non-negative tabindex) so taps on reaction badges won’t fire
onTap, or alternatively mark the reaction divs in MessageReactions with
role="button" and keep the selector to include [role="button"] so the existing
guard covers them.
---
Nitpick comments:
In `@src/components/message/CollapsibleMessage.tsx`:
- Line 79: The useImperativeHandle call currently has no dependency array which
recreates the handle every render and risks stale closure; update the code so
the exposed toggle is stable by either memoizing the toggleExpanded function
with useCallback (e.g., wrap toggleExpanded in useCallback and then call
useImperativeHandle(ref, () => ({ toggle: toggleExpanded }), [toggleExpanded]))
or implement toggleExpanded as a stable function using a ref + functional state
update and pass an empty or appropriate dependency array to useImperativeHandle;
reference the useImperativeHandle and toggleExpanded symbols when making the
change.
In `@tests/components/CollapsibleMessage.test.tsx`:
- Around line 17-28: The test sets a custom descriptor on
HTMLElement.prototype.scrollHeight but only resets mockScrollHeight in
afterEach, leaving the descriptor defined and causing test pollution; capture
the original descriptor (e.g., const originalScrollHeightDesc =
Object.getOwnPropertyDescriptor(HTMLElement.prototype, "scrollHeight")) before
calling Object.defineProperty in the setup and then restore it in afterEach
(either by re-defining the original descriptor if it exists or deleting the
property to fall back to the prototype chain) so that tests clean up the
prototype change; reference the existing mockScrollHeight and the prototype
modification to find where to save/restore the descriptor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4eb0a5b4-c635-4c14-86cd-4f7dcd90be4d
📒 Files selected for processing (6)
src/components/message/CollapsibleMessage.tsxsrc/components/message/MessageActions.tsxsrc/components/message/MessageItem.tsxsrc/components/message/SwipeableMessage.tsxsrc/index.csstests/components/CollapsibleMessage.test.tsx
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/components/message/SwipeableMessage.tsx (1)
35-37:⚠️ Potential issue | 🟡 MinorBroaden the tap guard beyond buttons and links.
childrenis arbitraryReactNode, so limiting the exclusion tobutton,[role='button'], andastill lets taps on other interactive descendants toggle collapse/expand. This is the same root issue as the earlier tap-conflict comment, just with fewer remaining cases now that reaction chips are native buttons.🛡️ Suggested fix
- const touchStartTargetRef = useRef<EventTarget | null>(null); + const touchStartTargetRef = useRef<Element | null>(null); ... - touchStartTargetRef.current = e.target; + touchStartTargetRef.current = + e.target instanceof Element ? e.target : e.currentTarget; ... - const target = touchStartTargetRef.current as Element | null; - if (!target?.closest("button, [role='button'], a")) { + if ( + !touchStartTargetRef.current?.closest( + "button, [role='button'], a[href], input, textarea, select, summary, [tabindex]:not([tabindex='-1'])", + ) + ) { onTap(); }Also applies to: 121-125, 145-148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/message/SwipeableMessage.tsx` around lines 35 - 37, The tap guard currently only excludes button, [role='button'], and a elements, so update the touch handlers (the functions using touchStartTargetRef, touchStartPosRef, and hasMovedRef in SwipeableMessage.tsx) to detect any interactive descendant: create a small helper (e.g., isInteractiveElement) that checks if the event target is an HTMLElement and uses closest()/matches() for a broader selector like "a, button, input, textarea, select, summary, [role], [tabindex]" or checks for focusable/tabbable attributes, then early-return from the tap/click collapse logic when that helper returns true; replace the current tag-specific checks in all handlers that reference touchStartTargetRef (also update the corresponding logic at the other occurrences you noted around lines ~121-125 and ~145-148).
🧹 Nitpick comments (1)
src/components/message/MessageReactions.tsx (1)
54-68: Expose the reaction chip as an actual toggle control.Now that each chip is a single
<button>, it should expose its pressed state explicitly. Right now the hidden×still contributes to the accessible name, whiletitleis hover-only and not reliably announced on touch/screen readers.♿ Suggested fix
<button key={emoji} type="button" className="bg-discord-dark-300 hover:bg-discord-dark-200 text-white px-1.5 py-0.5 rounded text-xs flex items-center gap-1 transition-colors cursor-pointer group" title={`${emoji} ${reactionData.count} ${reactionData.count === 1 ? "reaction" : "reactions"} by ${reactionData.users.join(", ")}`} + aria-pressed={reactionData.currentUserReacted} + aria-label={`${reactionData.currentUserReacted ? "Remove" : "Add"} ${emoji} reaction. ${reactionData.count} ${reactionData.count === 1 ? "reaction" : "reactions"}.`} onClick={() => onReactionClick(emoji, reactionData.currentUserReacted) } > <span>{emoji}</span> <span className="text-xs font-medium">{reactionData.count}</span> {reactionData.currentUserReacted && ( - <span className="ml-1 text-red-400 opacity-0 group-hover:opacity-100 transition-opacity text-xs"> + <span + aria-hidden="true" + className="ml-1 text-red-400 opacity-0 group-hover:opacity-100 transition-opacity text-xs" + > × </span> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/message/MessageReactions.tsx` around lines 54 - 68, The reaction chip should be an explicit toggle control: in the MessageReactions component, set the button's aria-pressed to reactionData.currentUserReacted and replace the hover-only title with an aria-label containing the emoji, count and users (so screen readers get the same info); also mark the decorative × span as aria-hidden="true" so it doesn't become part of the accessible name. Ensure onReactionClick(emoji, reactionData.currentUserReacted) remains the click handler and that the semantic button now correctly exposes pressed state and a persistent accessible label.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/components/message/SwipeableMessage.tsx`:
- Around line 35-37: The tap guard currently only excludes button,
[role='button'], and a elements, so update the touch handlers (the functions
using touchStartTargetRef, touchStartPosRef, and hasMovedRef in
SwipeableMessage.tsx) to detect any interactive descendant: create a small
helper (e.g., isInteractiveElement) that checks if the event target is an
HTMLElement and uses closest()/matches() for a broader selector like "a, button,
input, textarea, select, summary, [role], [tabindex]" or checks for
focusable/tabbable attributes, then early-return from the tap/click collapse
logic when that helper returns true; replace the current tag-specific checks in
all handlers that reference touchStartTargetRef (also update the corresponding
logic at the other occurrences you noted around lines ~121-125 and ~145-148).
---
Nitpick comments:
In `@src/components/message/MessageReactions.tsx`:
- Around line 54-68: The reaction chip should be an explicit toggle control: in
the MessageReactions component, set the button's aria-pressed to
reactionData.currentUserReacted and replace the hover-only title with an
aria-label containing the emoji, count and users (so screen readers get the same
info); also mark the decorative × span as aria-hidden="true" so it doesn't
become part of the accessible name. Ensure onReactionClick(emoji,
reactionData.currentUserReacted) remains the click handler and that the semantic
button now correctly exposes pressed state and a persistent accessible label.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29550a54-7f29-495e-8e03-281757df5df6
📒 Files selected for processing (2)
src/components/message/MessageReactions.tsxsrc/components/message/SwipeableMessage.tsx
ValwareIRC
approved these changes
Mar 10, 2026
This was referenced Mar 10, 2026
Merged
Merged
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.
added Small animations for hover buttons
In mobile chevron down is always shown
in mobile tapping a message also expands it
in desktop chevron down is only shown on hover together with other buttons with its own animation.
Fix: Expanding a message when in the bottom of the chat area scroll automatically scrolls down to keep it stuck to the bottom.
Summary by CodeRabbit
New Features
Improvements