Sidebar Update: Share and Notifications#2084
Merged
Conversation
Signed-off-by: prxt6529 <prxt@6529.io>
|
Warning Rate limit exceeded
⌛ 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 (1)
📝 WalkthroughWalkthroughExports HeaderQRModal; adds Notifications nav item with unread indicator and integrates desktop share modal into user menu; renames HeaderUserProxyDropdown → HeaderUserMenuDropdown with onOpenShare prop and markup changes; introduces useIsMobileDeviceStatus hook; updates tests and minor CSS/class tweaks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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.
🧹 Nitpick comments (2)
components/layout/sidebar/WebSidebarUser.tsx (1)
154-156: Use optional chaining for cleaner access.Same pattern as in
HeaderUserProxyDropdown.tsx- static analysis suggests optional chaining for improved readability.♻️ Suggested change
const hasUnreadOnOtherConnectedProfiles = connectedAccounts.some( (account) => !account.isActive && - ((connectedAccountUnreadNotifications ?? {})[ - account.address.toLowerCase() - ] ?? 0) > 0 + (connectedAccountUnreadNotifications?.[account.address.toLowerCase()] ?? + 0) > 0 );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/layout/sidebar/WebSidebarUser.tsx` around lines 154 - 156, The conditional accessing unread notifications uses manual null-coalescing and bracket lookup; replace it with optional chaining to simplify and mirror HeaderUserProxyDropdown.tsx — e.g., read the count via connectedAccountUnreadNotifications?.[account.address.toLowerCase()] ?? 0 and then compare > 0; update the expression in WebSidebarUser.tsx where connectedAccountUnreadNotifications and account.address are referenced to use optional chaining for cleaner, safer access.components/header/user/proxy/HeaderUserProxyDropdown.tsx (1)
142-144: Use optional chaining for cleaner access.Static analysis flagged this pattern. Optional chaining is more idiomatic and concise.
♻️ Suggested change
unreadNotificationsCount: - (connectedAccountUnreadNotifications ?? {})[ - account.address.toLowerCase() - ] ?? 0, + connectedAccountUnreadNotifications?.[ + account.address.toLowerCase() + ] ?? 0,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header/user/proxy/HeaderUserProxyDropdown.tsx` around lines 142 - 144, Replace the manual nullish-coalesce-to-empty-object then bracket-lookup on connectedAccountUnreadNotifications with optional chaining: access connectedAccountUnreadNotifications using optional chaining when indexing by account.address.toLowerCase(), and keep the existing fallback to 0; update the expression in HeaderUserProxyDropdown (referencing connectedAccountUnreadNotifications and account.address) accordingly.
🤖 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/header/user/proxy/HeaderUserProxyDropdown.tsx`:
- Around line 142-144: Replace the manual nullish-coalesce-to-empty-object then
bracket-lookup on connectedAccountUnreadNotifications with optional chaining:
access connectedAccountUnreadNotifications using optional chaining when indexing
by account.address.toLowerCase(), and keep the existing fallback to 0; update
the expression in HeaderUserProxyDropdown (referencing
connectedAccountUnreadNotifications and account.address) accordingly.
In `@components/layout/sidebar/WebSidebarUser.tsx`:
- Around line 154-156: The conditional accessing unread notifications uses
manual null-coalescing and bracket lookup; replace it with optional chaining to
simplify and mirror HeaderUserProxyDropdown.tsx — e.g., read the count via
connectedAccountUnreadNotifications?.[account.address.toLowerCase()] ?? 0 and
then compare > 0; update the expression in WebSidebarUser.tsx where
connectedAccountUnreadNotifications and account.address are referenced to use
optional chaining for cleaner, safer access.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3c94885-0e20-4f25-b011-fdc478f1113d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
components/header/share/HeaderShare.tsxcomponents/header/user/proxy/HeaderUserProxyDropdown.tsxcomponents/layout/sidebar/WebSidebar.tsxcomponents/layout/sidebar/WebSidebarNav.tsxcomponents/layout/sidebar/WebSidebarUser.tsx
Signed-off-by: prxt6529 <prxt@6529.io>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
__tests__/components/header/user/HeaderUserMenuDropdown.test.tsx (1)
161-167:⚠️ Potential issue | 🟠 MajorThe failure-path assertion is wrong.
HeaderUserMenuDropdown.runMenuAction()only callsonClose()after a successful action. WhenseizeSwitchConnectedAccountthrows, the component keeps the menu open and only shows the error toast, so the expectation on Line 166 will fail.Suggested fix
await waitFor(() => { expect(seizeSwitchConnectedAccount).toHaveBeenCalledWith("0xdef"); expect(setToast).toHaveBeenCalledWith( expect.objectContaining({ type: "error" }) ); - expect(onClose).toHaveBeenCalled(); + expect(onClose).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/header/user/HeaderUserMenuDropdown.test.tsx` around lines 161 - 167, The test's failure-path assertion incorrectly expects onClose() to be called; update the assertion in HeaderUserMenuDropdown.test for the error case where seizeSwitchConnectedAccount throws to assert that setToast was called with an error and that onClose was NOT called (or simply remove the onClose assertion). Locate the test around the waitFor block that calls runMenuAction and change the expectation referencing onClose to expect(onClose).not.toHaveBeenCalled() (or remove it) while keeping the existing expect(seizeSwitchConnectedAccount).toHaveBeenCalledWith("0xdef") and expect(setToast).toHaveBeenCalledWith(expect.objectContaining({ type: "error" })).
🧹 Nitpick comments (1)
__tests__/components/header/user/HeaderUserMenuDropdown.test.tsx (1)
40-71: Add coverage for the new Share branch.This helper never passes
onOpenShare, so the newly added Share action inHeaderUserMenuDropdown.tsxis still untested here. A small case for render/click behavior would catch regressions in one of the main features of this PR.Example extension
function renderDropdown(options: any) { mockConnect.mockReturnValue({ address: options.address, @@ render( <AuthContext.Provider value={authValue}> <HeaderUserMenuDropdown isOpen profile={options.profile} onClose={onClose} + onOpenShare={options.onOpenShare} /> </AuthContext.Provider> ); return { onClose, ...authValue, ...mockConnect.mock.results[0].value }; } + +it("calls onOpenShare when the Share action is clicked", () => { + const onOpenShare = jest.fn(); + renderDropdown({ + profile: profileBase, + address: "0xabc", + isConnected: true, + onOpenShare, + }); + + fireEvent.click(screen.getByRole("button", { name: "Share" })); + expect(onOpenShare).toHaveBeenCalled(); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/header/user/HeaderUserMenuDropdown.test.tsx` around lines 40 - 71, The test helper renderDropdown currently never supplies onOpenShare, so add an onOpenShare mock to the returned props and to the rendered HeaderUserMenuDropdown invocation: in renderDropdown, create const onOpenShare = jest.fn(); pass onOpenShare to <HeaderUserMenuDropdown ... onOpenShare={onOpenShare} />, include onOpenShare in the returned object, and add/modify tests to call the Share action (e.g., find the Share button and simulate click) and assert onOpenShare was called; this targets the renderDropdown helper and HeaderUserMenuDropdown share branch.
🤖 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/layout/sidebar/WebSidebarUser.tsx`:
- Around line 37-39: The desktop-share flag can be true briefly because
useIsMobileDevice() initializes false; update the logic in WebSidebarUser.tsx so
canUseDesktopShare only becomes true after device detection has settled: either
change useIsMobileDevice to return a tri-state (e.g. undefined | boolean) and
compute canUseDesktopShare = !capacitor.isCapacitor && isMobileDevice === false,
or add a separate isDeviceDetectionResolved boolean from the hook and compute
canUseDesktopShare = !capacitor.isCapacitor && isDeviceDetectionResolved &&
!isMobileDevice; update any UI that relies on canUseDesktopShare to respect the
resolved check so the Share action and QR modal are not shown until detection
completes.
---
Outside diff comments:
In `@__tests__/components/header/user/HeaderUserMenuDropdown.test.tsx`:
- Around line 161-167: The test's failure-path assertion incorrectly expects
onClose() to be called; update the assertion in HeaderUserMenuDropdown.test for
the error case where seizeSwitchConnectedAccount throws to assert that setToast
was called with an error and that onClose was NOT called (or simply remove the
onClose assertion). Locate the test around the waitFor block that calls
runMenuAction and change the expectation referencing onClose to
expect(onClose).not.toHaveBeenCalled() (or remove it) while keeping the existing
expect(seizeSwitchConnectedAccount).toHaveBeenCalledWith("0xdef") and
expect(setToast).toHaveBeenCalledWith(expect.objectContaining({ type: "error"
})).
---
Nitpick comments:
In `@__tests__/components/header/user/HeaderUserMenuDropdown.test.tsx`:
- Around line 40-71: The test helper renderDropdown currently never supplies
onOpenShare, so add an onOpenShare mock to the returned props and to the
rendered HeaderUserMenuDropdown invocation: in renderDropdown, create const
onOpenShare = jest.fn(); pass onOpenShare to <HeaderUserMenuDropdown ...
onOpenShare={onOpenShare} />, include onOpenShare in the returned object, and
add/modify tests to call the Share action (e.g., find the Share button and
simulate click) and assert onOpenShare was called; this targets the
renderDropdown helper and HeaderUserMenuDropdown share branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb7ef656-3a52-4797-a7ff-b3f8d6a86262
📒 Files selected for processing (7)
__tests__/components/header/user/HeaderUserMenuDropdown.test.tsx__tests__/components/header/user/HeaderUserProxyDropdownItem.test.tsxcomponents/header/user/HeaderUserMenuDropdown.tsxcomponents/header/user/HeaderUserProxyDropdownItem.tsxcomponents/layout/sidebar/WebSidebar.tsxcomponents/layout/sidebar/WebSidebarNav.tsxcomponents/layout/sidebar/WebSidebarUser.tsx
💤 Files with no reviewable changes (1)
- components/layout/sidebar/WebSidebarNav.tsx
✅ Files skipped from review due to trivial changes (1)
- tests/components/header/user/HeaderUserProxyDropdownItem.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- components/layout/sidebar/WebSidebar.tsx
|
simo6529
approved these changes
Mar 9, 2026
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.
Summary by CodeRabbit
New Features
Improvements
Tests