fix/ui fixes#160
Merged
Merged
Conversation
matheusfillipe
commented
Mar 12, 2026
| }; | ||
|
|
||
| const displayName = messageUser?.metadata?.["display-name"]?.value; | ||
| const username = message.userId.split("-")[0]; |
Contributor
Author
There was a problem hiding this comment.
@ValwareIRC I dont really know why we did this everywhere? So I am just removing...
|
Automated deployment preview for the PR in the Cloudflare Pages. |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces ReplyBadge with MessageReply, normalizes username handling to use full Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow as GitHub Actions Workflow
participant Compute as Compute step (job)
participant GitHubAPI as GitHub REST API
participant Publish as Publish Jobs
Workflow->>Compute: run compute previous release + generate notes
Compute->>GitHubAPI: GET tags/releases (find previous non-prerelease)
GitHubAPI-->>Compute: previous tag or none
alt previous tag found
Compute->>GitHubAPI: GET/POST generate-release-notes
GitHubAPI-->>Compute: release notes
else none
Compute-->>Compute: use "Initial release"
end
Compute->>Workflow: set output `release_body`
Workflow->>Publish: start publish jobs with `release_body` and make_latest derived from prerelease
Publish->>GitHubAPI: publish release(s) using `release_body`
GitHubAPI-->>Publish: publish response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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)
📝 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 |
![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: 3
🧹 Nitpick comments (3)
src/components/message/WhisperMessage.tsx (1)
116-126: Duplicate rendering logic for outgoing and incoming whispers.Both branches of the ternary render identical content. This can be simplified.
♻️ Suggested simplification
<span className="text-purple-300 font-normal"> - {isOutgoing ? ( - <> - from <span className="font-semibold">{sender}</span> to{" "} - <span className="font-semibold">{recipient}</span> - </> - ) : ( - <> - from <span className="font-semibold">{sender}</span> to{" "} - <span className="font-semibold">{recipient}</span> - </> - )} + from <span className="font-semibold">{sender}</span> to{" "} + <span className="font-semibold">{recipient}</span> </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/message/WhisperMessage.tsx` around lines 116 - 126, The JSX in WhisperMessage (component WhisperMessage) contains a redundant ternary using isOutgoing that renders the same fragment for both branches; simplify by removing the conditional and render the single fragment once (the from <span className="font-semibold">{sender}</span> to <span className="font-semibold">{recipient}</span>) so you keep sender and recipient usage but eliminate the duplicated branches and any unused isOutgoing-specific code.src/components/ui/UserSettings.tsx (1)
1326-1326: Minor inconsistency in Save button labels.Mobile view shows "Save" (line 1326) while desktop view shows "Save Changes" (line 1459) when there are unsaved changes. Consider aligning these for consistency.
✨ Optional: Align button labels
// Mobile (line 1326) - {hasUnsavedChanges ? "Save" : "No Changes"} + {hasUnsavedChanges ? "Save Changes" : "No Changes"}Or shorten desktop to match mobile if space is a concern.
Also applies to: 1459-1459
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/UserSettings.tsx` at line 1326, The Save button label is inconsistent between mobile and desktop in the UserSettings component: update the conditional rendering that uses hasUnsavedChanges so both mobile and desktop views display the same text (either "Save" or "Save Changes"); locate the JSX in UserSettings.tsx where hasUnsavedChanges is used (the mobile instance around the earlier conditional and the desktop instance around the later conditional) and change one of them to match the other so the label is consistent across breakpoints.src/components/message/MessageItem.tsx (1)
442-446: Measure the actual toolbar bounds instead of hard-coding them.The overlap detector assumes a single
90x32bottom-right toolbar, but this component now rendersMessageActionsin multiple layouts and with an optional delete action. That makes the collision box drift from the real UI and can misclassify copy buttons. Prefer reading the rendered toolbar’sgetBoundingClientRect()from a ref/data attribute.Also applies to: 1060-1111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/message/MessageItem.tsx` around lines 442 - 446, The hard-coded toolbar bounds (toolbarTop, toolbarLeft, toolbarRight, toolbarBottom) based on a fixed 90x32 box are incorrect; instead obtain the actual toolbar DOM rect from the rendered MessageActions element (use a ref or data-attribute and call getBoundingClientRect()) and compute overlap against that rect and msgRect. Update the overlap-detection logic where toolbarTop/Left/Right/Bottom are computed (and the similar logic around lines 1060-1111) to read toolbarRect = toolbarRef.current.getBoundingClientRect(), then use toolbarRect.top/left/right/bottom for collision checks; ensure the ref is attached to the MessageActions root and handle missing ref defensively (fallback to previous msgRect-based calculation).
🤖 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 45-60: The effect in useLayoutEffect (which reads
contentRef.current.scrollHeight and updates setContentHeight,
setCollapsedMaxHeight, setNeedsCollapsing and calls onNeedsCollapsing) must
re-run when the rendered content changes; add the rendered content value (the
prop/state named content) to the dependency array alongside maxLines and
onNeedsCollapsing so the measurement is recalculated whenever content updates.
In `@src/components/message/MessageItem.tsx`:
- Around line 492-505: The PM and channel user lookups use strict equality
against message.userId which causes casing mismatches; update the privateChat
and channel?.users.find predicates to normalize both sides (e.g., compare
lowercased or otherwise normalized username and message.userId) similar to the
no-channel fallback so that privateChat, channel user resolution, and resulting
userKey no longer fall back to "none" due to case differences; ensure you change
the predicates used in server?.privateChats?.find and channel?.users.find (and
any user variable comparisons) to use the same normalization function.
In `@src/components/ui/ReactionPopover.tsx`:
- Around line 61-79: The wheel handler (handleWheel) currently calls
e.preventDefault() which blocks native scrolling and prevents the chat's
useScrollToBottom logic from detecting scrolls; change handleWheel to only
stopPropagation() when the event target is outside popoverRef.current (remove
e.preventDefault()), and update the wheel listener options to passive: true,
capture: true in the document.addEventListener and the matching
removeEventListener so the handler no longer prevents default scrolling while
still preventing the event from reaching parent bubble-phase listeners.
---
Nitpick comments:
In `@src/components/message/MessageItem.tsx`:
- Around line 442-446: The hard-coded toolbar bounds (toolbarTop, toolbarLeft,
toolbarRight, toolbarBottom) based on a fixed 90x32 box are incorrect; instead
obtain the actual toolbar DOM rect from the rendered MessageActions element (use
a ref or data-attribute and call getBoundingClientRect()) and compute overlap
against that rect and msgRect. Update the overlap-detection logic where
toolbarTop/Left/Right/Bottom are computed (and the similar logic around lines
1060-1111) to read toolbarRect = toolbarRef.current.getBoundingClientRect(),
then use toolbarRect.top/left/right/bottom for collision checks; ensure the ref
is attached to the MessageActions root and handle missing ref defensively
(fallback to previous msgRect-based calculation).
In `@src/components/message/WhisperMessage.tsx`:
- Around line 116-126: The JSX in WhisperMessage (component WhisperMessage)
contains a redundant ternary using isOutgoing that renders the same fragment for
both branches; simplify by removing the conditional and render the single
fragment once (the from <span className="font-semibold">{sender}</span> to <span
className="font-semibold">{recipient}</span>) so you keep sender and recipient
usage but eliminate the duplicated branches and any unused isOutgoing-specific
code.
In `@src/components/ui/UserSettings.tsx`:
- Line 1326: The Save button label is inconsistent between mobile and desktop in
the UserSettings component: update the conditional rendering that uses
hasUnsavedChanges so both mobile and desktop views display the same text (either
"Save" or "Save Changes"); locate the JSX in UserSettings.tsx where
hasUnsavedChanges is used (the mobile instance around the earlier conditional
and the desktop instance around the later conditional) and change one of them to
match the other so the label is consistent across breakpoints.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02bcdf12-d50a-456d-86ae-4310e3665b69
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (29)
.github/workflows/publish.yamlpackage.jsonsrc/components/layout/ChatArea.tsxsrc/components/message/ActionMessage.tsxsrc/components/message/CollapsibleMessage.tsxsrc/components/message/EventMessage.tsxsrc/components/message/InviteMessage.tsxsrc/components/message/MessageActions.tsxsrc/components/message/MessageAvatar.tsxsrc/components/message/MessageHeader.tsxsrc/components/message/MessageItem.tsxsrc/components/message/MessageReactions.tsxsrc/components/message/MessageReply.tsxsrc/components/message/SwipeableMessage.tsxsrc/components/message/WhisperMessage.tsxsrc/components/ui/ReactionPopover.tsxsrc/components/ui/ReplyBadge.tsxsrc/components/ui/ScrollToBottomButton.tsxsrc/components/ui/UserProfileModal.tsxsrc/components/ui/UserSettings.tsxsrc/index.csssrc/lib/eventGrouping.tssrc/lib/ircUtils.tsxsrc/store/index.tstailwind.config.jstests/components/CollapsibleMessage.test.tsxtests/components/MessageHeader.test.tsxtests/components/MetadataDisplay.test.tsxtests/lib/ircMarkdown.test.ts
💤 Files with no reviewable changes (2)
- src/components/ui/ReplyBadge.tsx
- src/store/index.ts
Comment on lines
+45
to
+60
| useLayoutEffect(() => { | ||
| if (!contentRef.current) return; | ||
|
|
||
| const element = contentRef.current; | ||
| const computedStyle = window.getComputedStyle(element); | ||
| const lineHeight = Number.parseFloat(computedStyle.lineHeight) || 16; | ||
| const maxHeight = lineHeight * maxLines; | ||
| const element = contentRef.current; | ||
| const computedStyle = window.getComputedStyle(element); | ||
| const lineHeight = Number.parseFloat(computedStyle.lineHeight) || 16; | ||
| const maxHeight = lineHeight * maxLines; | ||
|
|
||
| const fullHeight = element.scrollHeight; | ||
| setContentHeight(fullHeight); | ||
| setCollapsedMaxHeight(`${lineHeight * maxLines}px`); | ||
| const fullHeight = element.scrollHeight; | ||
| setContentHeight(fullHeight); | ||
| setCollapsedMaxHeight(`${lineHeight * maxLines}px`); | ||
|
|
||
| const needs = fullHeight > maxHeight; | ||
| setNeedsCollapsing(needs); | ||
| onNeedsCollapsing?.(needs); | ||
| }, [maxLines, onNeedsCollapsing]); | ||
| const needs = fullHeight > maxHeight; | ||
| setNeedsCollapsing(needs); | ||
| onNeedsCollapsing?.(needs); | ||
| }, [maxLines, onNeedsCollapsing]); |
Contributor
There was a problem hiding this comment.
Missing content in useLayoutEffect dependency array.
The effect measures contentRef.current.scrollHeight which depends on the rendered content. If content changes without maxLines or onNeedsCollapsing changing, the collapse state won't be recalculated.
🔧 Suggested fix
}, [maxLines, onNeedsCollapsing]);
+ // Note: content is intentionally omitted to avoid re-measuring on every content change.
+ // The parent component should remount CollapsibleMessage if content changes significantly.Or if content changes should trigger recalculation:
- }, [maxLines, onNeedsCollapsing]);
+ }, [content, maxLines, onNeedsCollapsing]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/message/CollapsibleMessage.tsx` around lines 45 - 60, The
effect in useLayoutEffect (which reads contentRef.current.scrollHeight and
updates setContentHeight, setCollapsedMaxHeight, setNeedsCollapsing and calls
onNeedsCollapsing) must re-run when the rendered content changes; add the
rendered content value (the prop/state named content) to the dependency array
alongside maxLines and onNeedsCollapsing so the measurement is recalculated
whenever content updates.
Comment on lines
+492
to
505
| const privateChat = server?.privateChats?.find( | ||
| (pc) => pc.username === message.userId, | ||
| ); | ||
| if (privateChat) { | ||
| return `pm-${privateChat.id}`; | ||
| } | ||
| return "none"; | ||
| } | ||
|
|
||
| // For channels, find the user in the channel | ||
| const server = state.servers.find((s) => s.id === serverId); | ||
| const channel = server?.channels.find((c) => c.id === channelId); | ||
| const user = channel?.users.find( | ||
| (user) => user.username === message.userId.split("-")[0], | ||
| (user) => user.username === message.userId, | ||
| ); |
Contributor
There was a problem hiding this comment.
Normalize nickname matching in all lookup branches.
The selector is case-insensitive in the no-channel channel-user fallback, but these PM and channel lookups switch back to strict equality. If the stored nick casing differs from message.userId, userKey falls back to "none" and the avatar/display-name/metadata path stops resolving. Reuse the same normalization here as well.
Suggested fix
+ const normalizedUserId = message.userId.toLowerCase();
const privateChat = server?.privateChats?.find(
- (pc) => pc.username === message.userId,
+ (pc) => pc.username.toLowerCase() === normalizedUserId,
);
@@
const channel = server?.channels.find((c) => c.id === channelId);
const user = channel?.users.find(
- (user) => user.username === message.userId,
+ (user) => user.username.toLowerCase() === message.userId.toLowerCase(),
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/message/MessageItem.tsx` around lines 492 - 505, The PM and
channel user lookups use strict equality against message.userId which causes
casing mismatches; update the privateChat and channel?.users.find predicates to
normalize both sides (e.g., compare lowercased or otherwise normalized username
and message.userId) similar to the no-channel fallback so that privateChat,
channel user resolution, and resulting userKey no longer fall back to "none" due
to case differences; ensure you change the predicates used in
server?.privateChats?.find and channel?.users.find (and any user variable
comparisons) to use the same normalization function.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/publish.yaml:
- Around line 19-20: The collect-version job currently sets permissions.contents
to read which prevents the "Compute previous release tag and generate notes"
step from calling the REST endpoint; change the job's permissions block for the
collect-version job so contents is set to write (permissions: contents: write)
to allow POST /repos/{owner}/{repo}/releases/generate-notes to succeed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77fbb372-49a5-46a2-b86a-99ca87fda4e7
📒 Files selected for processing (2)
.github/workflows/publish.yamlbiome.json
✅ Files skipped from review due to trivial changes (1)
- biome.json
matheusfillipe
force-pushed
the
fix/ui-fixes
branch
from
882da91 to
05c6b74
Compare
ValwareIRC
approved these changes
Mar 12, 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
Bug Fixes
UI/UX Improvements
Tests
Chores