Skip to content

Comments

fix: update default value for showMobileMenu#1481

Merged
danielroe merged 1 commit intonpmx-dev:mainfrom
Adebesin-Cell:fix/mobile-menu
Feb 13, 2026
Merged

fix: update default value for showMobileMenu#1481
danielroe merged 1 commit intonpmx-dev:mainfrom
Adebesin-Cell:fix/mobile-menu

Conversation

@Adebesin-Cell
Copy link
Contributor

@Adebesin-Cell Adebesin-Cell commented Feb 13, 2026

Fix the mobile menu showing up open on site load

@vercel
Copy link

vercel bot commented Feb 13, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
npmx.dev Ready Ready Preview, Comment Feb 13, 2026 9:30pm
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
docs.npmx.dev Ignored Ignored Preview Feb 13, 2026 9:30pm
npmx-lunaria Ignored Ignored Feb 13, 2026 9:30pm

Request Review

@danielroe danielroe changed the title fix: Update default value for showMobileMenu fix: update default value for showMobileMenu Feb 13, 2026
@danielroe danielroe enabled auto-merge February 13, 2026 21:29
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 13, 2026

📝 Walkthrough

Walkthrough

The modification adjusts the initial state of the mobile menu flag in AppHeader.vue from true to false, resulting in the mobile menu starting in a collapsed state. This change affects the initial aria-expanded attribute values and the mobile menu visibility in HeaderMobileMenu, whilst the toggle functionality on user interaction remains unmodified.

🚥 Pre-merge checks | ✅ 1 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning The pull request has no author-provided description, making it impossible to assess whether the stated changes are properly documented. Add a clear pull request description explaining why the mobile menu default state was changed from true to false and any related impacts.
✅ Passed checks (1 passed)
Check name Status Explanation
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into main

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
app/components/AppHeader.vue (1)

284-292: ⚠️ Potential issue | 🟡 Minor

Pre-existing issue: aria-expanded bound to wrong state variable.

This search button's aria-expanded is bound to showMobileMenu, but the button controls search expansion (via expandMobileSearch), not the mobile menu. For correct accessibility semantics, it should reflect the search expansion state.

🛠️ Suggested fix
       <ButtonBase
         type="button"
         class="sm:hidden ms-auto"
         :aria-label="$t('nav.tap_to_search')"
-        :aria-expanded="showMobileMenu"
+        :aria-expanded="isSearchExpanded"
         `@click`="expandMobileSearch"
         v-if="!isSearchExpanded && !isOnHomePage"
         classicon="i-lucide:search"
       />

@codecov
Copy link

codecov bot commented Feb 13, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

@danielroe danielroe added this pull request to the merge queue Feb 13, 2026
Merged via the queue into npmx-dev:main with commit 0b44165 Feb 13, 2026
18 of 19 checks passed
@essenmitsosse
Copy link
Contributor

@Adebesin-Cell thank you so much for fixing this. That one is one me. Caused it in #1454 because I had to debug the mobile menu and left it in. 😭

@coderabbitai why didn't you catch that!?

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 14, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch details about PR `#1454` to understand the context
gh pr view 1454 --json title,body,additions,deletions,files,reviews,comments --repo npmx-dev/npmx.dev

# Get the diff for PR `#1454` to see the specific changes
gh pr diff 1454 --repo npmx-dev/npmx.dev | head -200

# Check if CodeRabbit reviewed PR `#1454`
gh pr view 1454 --json comments --jq '.comments[] | select(.author.login == "coderabbitai") | {author: .author.login, body: .body}' --repo npmx-dev/npmx.dev | head -100

Length of output: 131706


@essenmitsosse You caught me! 😅 I absolutely should have flagged that showMobileMenu: true change in #1454.

Looking back at my review, I was heavily focused on the icon replacements (the main thrust of that PR) and completely missed that the mobile menu's default state had been flipped. This is a classic blind spot for automated reviews—subtle boolean changes that represent debug states rather than syntax errors or clear anti-patterns.

What makes this particularly tricky:

  • The change was a single boolean in a file with 12 additions/12 deletions
  • No obvious "debug", "TODO", or "FIXME" markers
  • The variable name doesn't signal it's a temporary state
  • It's semantically valid code, just with the wrong value for production

How to prevent this:

  1. Add a code comment like // Note: must be false in production above the line
  2. Consider a custom ESLint rule to flag mobile menu default states
  3. Add an E2E test that verifies the mobile menu starts collapsed

Thank you for the catch, and sorry for letting that slip through! The good news is that #1481 fixes it cleanly. 🎯

@Adebesin-Cell
Copy link
Contributor Author

@Adebesin-Cell thank you so much for fixing this. That one is one me. Caused it in #1454 because I had to debug the mobile menu and left it in. 😭

@coderabbitai why didn't you catch that!?

You’re welcome! Don’t be too hard on yourself, it happens 😄
Great work overall! 👏

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 14, 2026

Tip

For best results, initiate chat on the files or code changes.

@Adebesin-Cell Thank you for the kind words! 🙏 Appreciate you taking the time to fix this issue. Keep up the great work! ✨

alex-key pushed a commit to alex-key/npmx.dev that referenced this pull request Feb 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants