fix: mobile horizontal sub-nav never rendering in AppNavBar - #5411
Open
zigzagdev wants to merge 2 commits into
Open
fix: mobile horizontal sub-nav never rendering in AppNavBar#5411zigzagdev wants to merge 2 commits into
AppNavBar#5411zigzagdev wants to merge 2 commits into
Conversation
…ng it secondaryMenu/desktopSubNavPosition/mobileSubNavPosition were plain lets mutated as a side effect of the desktop mainItems.map(), which runs after the mobile block's JSX (evaluated earlier in the same return statement) already read those variables. The mobile horizontal sub-nav condition therefore always saw them at their initial values and could never render, which is what the "always false" ts-expect-error on that line was masking. Compute the active item's secondary menu and positioning up front with useMemo/find so both the mobile and desktop blocks read the same, already-resolved values.
Regression test for the active-item eval-order bug: an item whose navPosition.mobile is 'horizontal' (and desktop is 'vertical') must render exactly one "Secondary navigation" nav, on the mobile side.
zigzagdev
requested review from
DianaSuvorova,
dyesin,
hualf1995 and
lijim
as code owners
September 6, 2026 08:13
zigzagdev
changed the title
AppNavBar
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
1 participant
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.
Fixes #1, Fixes #2Description
Fixing an evaluation-order bug in
AppNavBarwheresecondaryMenu,desktopSubNavPosition, andmobileSubNavPositionwere plainletvariables mutated as a side effect inside the desktopmainItems.map().Because the mobile nav block is evaluated earlier in the same JSX return statement, it always read these variables before that map ran, so
mobileSubNavPosition === POSITION.horizontalcould never be true and the mobile horizontal sub-nav never rendered (a// @ts-expect-error always falsecomment in the source acknowledged this).Now the active item's secondary menu and positioning are resolved up front via
useMemo/find, so both the mobile and desktop blocks read the same, already-resolved values.Scope