Skip to content

Conversation

lidongcheng88
Copy link
Contributor

@lidongcheng88 lidongcheng88 commented Oct 7, 2025

Summary by CodeRabbit

  • Bug Fixes
    • Corrected auto-collapse behavior in mixed-navigation layouts. The sidebar now automatically collapses when using either header-mixed-nav or sidebar-mixed-nav, aligning behavior across layout variants and improving responsiveness on smaller viewports.
    • No changes to configuration or other layout interactions; existing behavior remains the same elsewhere.
    • Ensures a more predictable navigation experience when switching between mixed-nav layouts.

Copy link

changeset-bot bot commented Oct 7, 2025

⚠️ No Changeset found

Latest commit: a86197b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

coderabbitai bot commented Oct 7, 2025

Walkthrough

Expanded the auto-collapse trigger to run when the app layout is either header-mixed-nav or sidebar-mixed-nav by replacing a strict equality check with an inclusion check. No other logic, structure, or public APIs were modified.

Changes

Cohort / File(s) Summary
Layout auto-collapse condition update
packages/effects/layouts/src/basic/layout.vue
Broadened condition: replaces layout === 'sidebar-mixed-nav' with inclusion check for ['header-mixed-nav','sidebar-mixed-nav'] to trigger auto-collapse in both scenarios; no other logic changes; no public/exported API changes.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant U as User
  participant L as BasicLayout
  participant C as Config/Store
  participant S as Sidebar

  U->>L: Load/Resize/Init
  L->>C: Read current appLayout
  alt appLayout in ['header-mixed-nav','sidebar-mixed-nav']  (new)
    L->>S: autoCollapse()
    S-->>L: collapsed
  else Other layouts
    L-->>U: No auto-collapse
  end
  note over L,S: Logic otherwise unchanged
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I hop through layouts, ears alert and wide,
Two mixed-nav paths now trigger a tidy slide.
With a flick of a paw, sidebars softly fold,
The warren stays neat, just as stories told.
Thump-thump—clean flow, in burrows of code.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning There is no pull request description provided, so the required template sections—including a summary of the change, type of change, and checklist items—are entirely missing. Please fill out the PR description using the repository template by adding a detailed “Description” section, selecting the appropriate “Type of change,” and completing the “Checklist” to confirm tests, documentation, and style guidelines are addressed.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly indicates that this change fixes automatic closing of the secondary sidebar in the mixed-layout scenario, which aligns with the update to collapse logic for mixed modes; it directly references the actual behavior being corrected without extraneous detail.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

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.

Actionable comments posted: 0

🧹 Nitpick comments (1)
packages/effects/layouts/src/basic/layout.vue (1)

161-163: Logic change looks good, but consider using computed properties for consistency.

The expansion to include both header-mixed-nav and sidebar-mixed-nav correctly aligns with the PR objective.

For better maintainability and consistency with the rest of the codebase (see lines 46-47, 94), consider using the existing computed properties instead of raw string comparison:

-    ['header-mixed-nav', 'sidebar-mixed-nav'].includes(
-      preferences.app.layout,
-    ) &&
+    (isHeaderMixedNav.value || isSideMixedNav.value) &&

This approach is more resilient to potential changes in layout string values and follows the pattern used elsewhere in the file.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2ce161e and a86197b.

📒 Files selected for processing (1)
  • packages/effects/layouts/src/basic/layout.vue (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: post-update (ubuntu-latest)
  • GitHub Check: post-update (windows-latest)
🔇 Additional comments (1)
packages/effects/layouts/src/basic/layout.vue (1)

173-175: Verify: Should auto-collapse also trigger on route changes?

The autoCollapseMenuByRouteMeta function is only called in onMounted, meaning it runs once when the component mounts. If a user navigates to a different route with hideInMenu: true after mount, the sidebar won't auto-collapse.

Is this mount-only behavior intentional, or should there be a route watcher to handle subsequent navigation? For example:

watch(
  () => route.path,
  () => {
    autoCollapseMenuByRouteMeta(route);
  }
);

Please confirm whether auto-collapse should respond to route changes or only on initial mount.

@jinmao88 jinmao88 merged commit 03ce030 into vbenjs:main Oct 13, 2025
5 checks passed
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.

2 participants