Uh oh!
There was an error while loading. Please reload this page.
fix(topnav): close open dropdown instantly when another tab is hovered (#320) - #371
Merged
Conversation
📝 WalkthroughWalkthroughDesktop navigation dropdown state is centralized in ChangesDesktop dropdown coordination
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant User
participant NavGroupTrigger
participant DesktopGroups
User->>NavGroupTrigger: Click or hover a group
NavGroupTrigger->>DesktopGroups: Invoke open or close callback
DesktopGroups->>NavGroupTrigger: Set the active group
User->>DesktopGroups: Click outside or press Escape
DesktopGroups->>NavGroupTrigger: Close the active panel
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 8adc755 | Commit Preview URL Branch Preview URL | Jul 29 2026, 09:58 AM |
AndresL230
marked this pull request as draft
July 27, 2026 23:50
#320) Rebase of PR #371 onto current main (the branch had drifted ~147 commits behind; its true diff touches only TopNav.tsx/TopNav.test.tsx, and main had not modified either file since the merge base, so the 3-way apply was conflict-free). Lift the per-trigger dropdown open-state into a new DesktopGroups row component that owns a single openIndex. Previously each NavGroupTrigger kept its own open flag and 140ms close-timer, so hovering from tab A to tab B cancelled only B's timer — A's panel lingered and two panels could show at once. With one owner, entering any tab replaces openIndex synchronously, closing the old panel instantly; the 140ms close-delay now only applies when the cursor leaves the row entirely. NavGroupTrigger becomes presentational (open/onOpen/onScheduleClose/ onClose props); route-change close, click-outside, and Escape handling move to the row level; blur-out of a trigger wrapper still closes immediately. Adds a regression test: opening Community must immediately close Learn (panel gone, aria-expanded flipped). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
AndresL230
marked this pull request as ready for review
July 29, 2026 09:43
AndresL230force-pushed
the
fix/320-topnav-menu-close
branch
from
July 29, 2026 09:43
441a711 to
57c426aCompareAndresL230
commented
Jul 29, 2026
Collaborator
Code reviewFound 1 issue:
Sapling/frontend/src/components/TopNav.tsx Lines 313 to 317 in 57c426a 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
…very trigger wrapper, not the flex:1 row (review finding) The lifted click-outside check guarded on rowRef, which stretches across the header's blank strip; a keyboard-opened panel (no hover timer armed) got stuck after a click there. Clicks now dismiss unless inside a [data-nav-group] wrapper. Adds the dead-space regression test plus a fake-timers test for the actual #320 hover-timer race. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
AndresL230
commented
Jul 29, 2026
Collaborator
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/TopNav.tsx (1)
394-399: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t close a trigger with the same activation that should open it
The button opens with
onFocus, but itsonClick={() => open ? onClose() : onOpen()}closes again once focus has setopentotrue. This also affects existing click tests that assume a click can leave a panel open. Make the click handler open-only instead of toggling.🐛 Proposed fix
- onClick={() => (open ? onClose() : onOpen())}+ onClick={onRead} onFocus={onOpen}(note: keep the existing open function name; the intent is
onClick={onOpen})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/TopNav.tsx` around lines 394 - 399, Update the trigger button’s onClick handler in the TopNav component to call onOpen directly instead of toggling between onOpen and onClose. Keep the existing onFocus={onOpen} behavior and preserve the onClose handler elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@frontend/src/components/TopNav.tsx`:
- Around line 394-399: Update the trigger button’s onClick handler in the TopNav
component to call onOpen directly instead of toggling between onOpen and
onClose. Keep the existing onFocus={onOpen} behavior and preserve the onClose
handler elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f104b3e-d484-46fc-910f-c35505a0f000
📒 Files selected for processing (2)
frontend/src/components/TopNav.test.tsxfrontend/src/components/TopNav.tsx
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
Closes#320.
Problem
Hovering slightly outside a top-nav tab (or moving from one tab to another) left the tab's subcategory panel open instead of closing, and could briefly show two panels at once.
Root cause
Each
NavGroupTriggerowned its ownopenstate and its own 140ms close-timer. Moving the cursor from tab A to tab B fired B'sonMouseEnter, which cancelled only B's timer — A's 140ms timer kept A's panel open, so both were visible during the overlap and the old one lingered.Fix
Lift the open-state into a single
DesktopGroupsowner tracking oneopenIndexfor the whole row. Entering any tab replacesopenIndexsynchronously, so the previously-open panel closes the instant the cursor reaches a new tab — at most one panel is ever open. Click-outside / Escape / route-change closing move up to the same owner. The 140ms grace delay is kept but now only applies when the cursor leaves the row entirely (its original anti-flicker purpose for the trigger↔panel handoff).Testing
no lingering second panel).tsc --noEmit: clean.eslint: 0 errors.vitest runcould not execute in this environment — vitest 4 + rolldown require Node 22 (per.nvmrc) and this machine has Node 20.12; CI runs on Node 22.🤖 Generated with Claude Code
Summary by CodeRabbit