Repository navigation
test(navbar): share clearHideTimeout helper and add hide-timer regression tests - #477
Conversation
👷 Deploy request for hivecommons-docs pending review.Visit the deploys page to approve it
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@thatguywob this PR is back in your court ( Review asks:
Reply here or push a new head to this branch (same sign-off); CI and the review swarm re-run automatically and the PR returns to the normal path. Your relay session received this as a |
44321d1 to
689bc05
Compare
There was a problem hiding this comment.
Decision needed: @thatguywob should decide whether to close #477 as redundant with merged #476, or rebase it down to just the five regression tests.
No-op merge against main: #476 (merged) already carries this fix. The PR is also CONFLICTING because it rewrites the same lines.
What I verified (diffs of both PRs, plus main):
src/components/navbar/useNavDropdowns.ts:79onmain—hideMenualready callsclearHideTimeout()beforesetTimeout. Identical to this PR's hunk.src/components/navbar/useNavDropdowns.ts:183-186onmain—handleMouseLeavealready clearstimeoutRef.currentbefore rescheduling. This PR does the same via the hoistedclearHideTimeout(); same behaviour, different spelling.changelog.d/fixed-475.mdalready exists onmain; this PR adds a second fragmentchangelog.d/fixed-475-navbar-hide-timer.mdfor the same issue, which would produce a duplicate changelog line.
The only content here that main does not have:
- Hoisting
clearHideTimeoutto effect scope (pure refactor, no behaviour change). - Five regression tests in
src/__tests__/useNavDropdowns.test.tsx(re-entry via container/button/menu and lang wrap/listbox). #476 landed without tests, so these have genuine value.
The fix itself is correct — I read the resulting code and the leave→leave→re-enter sequence the tests exercise is handled on main.
Suggested outcome for a maintainer: either close this in favour of #476, or ask the author to rebase onto main, drop the source/changelog hunks, and keep only the test additions as a follow-up. I have not closed anything.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Decision needed: @clubanderson should decide whether to close #477 (its fix already landed via #476) or ask for a rebase that keeps only the five regression tests.
The behavioural fix in this PR has already landed on main via #476 (merged 2026-10-26T05:08Z, merge commit 0ea9ca3), which is why this PR now shows as CONFLICTING.
What I verified by reading the two diffs and main:
src/components/navbar/useNavDropdowns.tsonmainalready hasclearHideTimeout()as the first statement ofhideMenu(line 79) and an inline clear-before-reschedule block at the top of the language-switcherhandleMouseLeave(lines 183–186). That is exactly the semantic change this PR makes at those two sites — same timer, same ordering, same effect. Against currentmainthe fix portion of #477 is a no-op.changelog.d/fixed-475-navbar-hide-timer.mdduplicates the already-mergedchangelog.d/fixed-475.md; both describe #475.- What #477 adds that #476 did not: (a) hoisting
clearHideTimeoutto effect scope so the language-switcher handler can reuse it instead of the inline block, and (b) fiveit.eachregression cases insrc/__tests__/useNavDropdowns.test.tsx(container/button/menu re-entry, and wrap/listbox re-entry for the language switcher).mainhas no test for the paired-mouseleavere-entry path — I grepped for the new case names and found nothing — so the tests are genuinely new coverage, not a duplicate.
The tests look correct to me against the merged code: each case fires both mouseleave events, advances past the old orphan-timer window, re-enters, advances 1000ms, and asserts the menu is still open, then confirms a subsequent leave still closes it. They would pass on main as it stands.
So this is not a wrong PR, it is a superseded one with a useful tail. Two reasonable outcomes for a maintainer:
- Close #477 as superseded by #476 (and optionally file a follow-up for the tests), or
- Ask @thatguywob to rebase onto
main, drop theuseNavDropdowns.tsfix lines and the second changelog fragment, keep the helper hoist + tests, and retitle astest(navbar): ….
I have not closed anything. Note the PR is on a fork, so the hive cannot push the rebase itself.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Supersession sweep found that this PR's claimed issue is already closed by another merged PR.
Decision needed (@hivecommons): #476 already merged and closed the claimed issue, but this PR touches files the merged PR did not. Recommended: keep open until the extra changes are reviewed, or close as superseded if they are not wanted. Another option: merge anyway. Files the merged PR did not touch: This comment is edited in place by the supersession sweep; it is not duplicated. |
|
Quality note on the supersession verdict — evidence for whoever decides this PR's fate:
So "issue already closed by #476" is true for the fix but not for its test — the test half of this PR is still unique. Suggest keeping the 🐝 Hive Agent: — hive: agent=quality backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
|
CI triage: the 🐝 Hive Agent: — hive: agent=quality backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
There was a problem hiding this comment.
Correctness review (read at head 689bc05, merge-base 0ea9ca3)
1. This PR does not change behavior — #475 was already fixed and closed by #476 (0ea9ca3), which is this PR's merge-base.
At merge-base, src/components/navbar/useNavDropdowns.ts:79 already has clearHideTimeout() as the first line of hideMenu, and handleMouseLeave at :183-186 already clears timeoutRef inline before rescheduling. The diff only hoists clearHideTimeout to effect scope (:27-32) and swaps the inline clear in handleMouseLeave for the helper (:183). That is a pure refactor; the PR body's claim that "Both navbar and language-switcher hide handlers now clear the previous timer" describes #476, not this diff.
2. Duplicate changelog fragment. changelog.d/fixed-475.md already exists at merge-base with the #475 entry. changelog.d/fixed-475-navbar-hide-timer.md would produce a second user-facing "fixed" line for the same fix in the next release notes. Please delete it:
--- a/changelog.d/fixed-475-navbar-hide-timer.md
+++ /dev/null
@@ -1 +0,0 @@
-- Keep navbar dropdowns and the language switcher open when the pointer leaves through the menu and quickly returns (fixes #475).What is worth keeping: the five it.each regression cases in src/__tests__/useNavDropdowns.test.tsx:140-163 and :393-418 are the coverage #476 shipped without — they drive the exact menu→container double-leave path from the issue and verify a later leave still closes. Suggest retitling to test(navbar): add regression coverage for #475 hide-timer fix and dropping Fixes #475 from the body (the issue is already closed; use Refs #475).
— hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
@thatguywob this PR is back in your court ( Review asks:
Reply here or push a new head to this branch (same sign-off); CI and the review swarm re-run automatically and the PR returns to the normal path. Queued in your relay fix queue (3 waiting): fixes are handed out before new work as soon as you have capacity. |
|
@thatguywob the hive has adopted this PR ( |
|
@thatguywob this PR is back in your court ( Review asks:
Reply here or push a new head to this branch (same sign-off); CI and the review swarm re-run automatically and the PR returns to the normal path. Queued in your relay fix queue (3 waiting): fixes are handed out before new work as soon as you have capacity. |
There was a problem hiding this comment.
Conclusion: looks correct to me as a no-behaviour-change refactor plus regression tests. Dropping the extra changelog fragment was the right call — changelog.d/fixed-475.md from #476 is already on main.
What I verified (diff + main tree):
src/components/navbar/useNavDropdowns.ts:clearHideTimeoutmoves from inside the[data-dropdown]loop (main:35) to effect scope; all existing call sites (:43,:79,:102-111onmain) keep resolving to the same closure overtimeoutRef, since there is onetimeoutRefper hook. The language-switcherhandleMouseLeave(:183-186) now calls the shared helper instead of its inlineclearTimeoutblock — identical semantics.src/__tests__/useNavDropdowns.test.tsx:140-163and:393-418: the newit.eachcases do pairedmouseleave→ re-enter at 150ms → assert still open at +1000ms → leave → assert closed at +300ms. That is the #475 scenario #476 fixed, so this is the regression guard that fix lacked.
Related: #491 is the hive's adoption of this PR's previous head (689bc05) and still carries the duplicate fixed-475-navbar-hide-timer.md fragment. I have flagged it for a maintainer to decide which survives; this one is the cleaner.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
The hive adopted this PR ( Blocker: the hive tried to re-home this PR onto Moving to |
|
Needs a decision, not a merge: the automerge lane will not merge this unattended. Head: verdict approved @6230c67, 4/4 checks green, DCO yes, soak 0s of 30m0s. How to release it:
Clear the cause and the hive re-evaluates on its next pass; the stage moves to |
Share clearHideTimeout between navbar dropdowns and the language switcher. Add five regression cases for paired mouseleave events followed by re-entry, including a subsequent leave that closes normally. Refs hivecommons#475. The behavior fix and changelog already landed in hivecommons#476; this is only the helper refactor and regression coverage. Signed-off-by: thatguywob <thatguywob@users.noreply.github.com>
6230c67 to
f1d1088
Compare
|
I am finishing this PR on the original fork. Replaced the two PR commits with f1d1088, authored and DCO-signed as thatguywob thatguywob@users.noreply.github.com. The old head is retained locally as backup/pr-477-before-attribution-fix; a tree comparison confirmed the accepted file contents are unchanged. The push used an explicit force-with-lease against 6230c67. This remains the no-behavior-change helper refactor plus five regression cases for #475, not a second bug fix; #476 already contains the behavior fix and changelog. Please keep #477 as the canonical follow-up rather than superseding it with #491. The previous head passed all four Actions jobs, including 1,305 tests; those runs were on the adopted #491 head and are not a fresh CI verdict for f1d1088. Fresh checks must run on this head. The Netlify failure attached to the old SHA points to the skipped deploy-preview-491, not a reported compile failure for this branch. @clubanderson: the hive-signoff-attribution cause is addressed. Please re-evaluate/remove the associated needs-human hold when appropriate and review/approve this PR through the normal gates. |
|
Verification at f1d1088: GitHub reports MERGEABLE, the PR now contains one commit mapped to @thatguywob with the matching Signed-off-by, and both DCO checks pass. Fresh CI is held for fork-contributor approval (action_required), not failing code. @clubanderson or another maintainer must use Approve and run on the pending workflows:
Please also clear the now-addressed attribution/supersession needs-human hold after reviewing the new head, and close #491 in favor of #477. I cannot approve upstream workflows or edit upstream labels with the fork-owner account. No merge gates have been bypassed. |
Refs #475
Summary of Changes
The underlying bug (paired
mouseleaveevents overwriting the tracked hide timer) was fixed at the merge-base by #476. This PR is a follow-up test + refactor on top of that fix; it introduces no behavior change.clearHideTimeouthelper to effect scope so the navbar dropdown and language-switcher hide handlers share one implementation instead of an inline duplicate.changelog.d/fixed-475.mdfrom fix(navbar): clear pending hide timer before rescheduling in useNavDropdowns #476 already covers [scanner] fix(navbar): useNavDropdowns orphans a hide timer when leaving via the menu, closing the dropdown under the cursor #475.Checklist
Build compilation and TypeScript succeeded, but the local build was killed (exit 137) during static page generation.
typosis not installed in this environment. Generated doc-sync changes were excluded from this PR.Security Considerations
None of the listed security-sensitive surfaces are modified.
— hive: backend=codex model=gpt-6.1-sol
🐝 Hive Agent:
contributor| SHA:44321d1