fix(tabber): change store key & preserve query params when switching tabbers #5500
Conversation
This change modifies the tabber store key from `fullPath` to `path`, impacting existing tabber records. ### Reason: Using `param` and `query` allows for more flexible tabber uniqueness: - When different parameters should be stored in the same tabber, use `query`. - When different parameters should be stored in separate tabbers, use `param`.
dingdayu
requested review from
anncwb,
vince292007 and
mynetfan
as code owners
|
WalkthroughThis pull request modifies the tab management system by enhancing the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
packages/@core/ui-kit/tabs-ui/src/components/tabs-chrome/tabs.vueOops! Something went wrong! :( ESLint: 9.19.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/node_modules/@vben/eslint-config/dist/index.mjs' imported from /eslint.config.mjs packages/@core/ui-kit/tabs-ui/src/components/tabs/tabs.vueOops! Something went wrong! :( ESLint: 9.19.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/node_modules/@vben/eslint-config/dist/index.mjs' imported from /eslint.config.mjs packages/effects/layouts/src/basic/tabbar/use-tabbar.tsOops! Something went wrong! :( ESLint: 9.19.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/node_modules/@vben/eslint-config/dist/index.mjs' imported from /eslint.config.mjs
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/effects/layouts/src/basic/tabbar/use-tabbar.ts(1 hunks)packages/stores/src/modules/tabbar.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test (windows-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: Lint (windows-latest)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: Lint (ubuntu-latest)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: post-update (windows-latest)
- GitHub Check: post-update (ubuntu-latest)
🔇 Additional comments (1)
packages/effects/layouts/src/basic/tabbar/use-tabbar.ts (1)
102-112: LGTM! Improved tab state management by watching both path and query.The change to watch both
route.pathandroute.queryensures that tabs are properly updated when either the path or query parameters change, improving the preservation of tab state.
| function getTabPath(tab: RouteRecordNormalized | TabDefinition) { | ||
| return decodeURIComponent((tab as TabDefinition).fullPath || tab.path); | ||
| return decodeURIComponent(tab.path); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM! Simplified tab path resolution.
The change to consistently use tab.path instead of fullPath makes the tab path resolution more predictable. However, since this is a breaking change:
Please verify the impact by checking existing tab state persistence:
🏁 Script executed:
#!/bin/bash
# Description: Check for any remaining uses of fullPath in tab-related code
# that might be affected by this change
echo "Checking for remaining fullPath usage in tab-related code..."
rg -l 'fullPath.*tab'
echo "Checking for direct tab.fullPath access..."
ast-grep --pattern 'tab.fullPath'Length of output: 834
Attention: Breaking Change Impact Detected
The update to use tab.path in the getTabPath function is not fully propagated across the codebase. Our automated checks revealed several remaining references to tab.fullPath which could lead to inconsistencies or runtime errors. Specifically:
- Files referencing
tab.fullPath:- packages/stores/src/modules/tabbar.ts:
- Line 200:
if (path !== tab.fullPath) { … } - Line 302:
openRouteInNewWindow(tab.fullPath || tab.path);
- Line 200:
- packages/stores/src/modules/tabbar.test.ts:
- Line 276: Using
tab.fullPathin a lookup.
- Line 276: Using
- Other components using
tab.fullPath:- packages/@core/ui-kit/tabs-ui/src/components/tabs-chrome/tabs.vue
- packages/@core/ui-kit/tabs-ui/src/components/tabs/tabs.vue
- packages/effects/layouts/src/basic/tabbar/use-tabbar.ts:149 (used with router.push)
- packages/stores/src/modules/tabbar.ts:
These findings confirm a breaking change impact that needs careful attention. Please review and update these references to ensure consistency across the system.
There was a problem hiding this comment.
packages/effects/layouts/src/basic/tabbar/use-tabbar.ts:149 和 packages/stores/src/modules/tabbar.ts:302
{
handler: async () => {
if (!contentIsMaximize.value) {
await router.push(tab.fullPath);
}
toggleMaximize();
},
icon: contentIsMaximize.value ? Minimize2 : Fullscreen,
key: contentIsMaximize.value ? 'restore-maximize' : 'maximize',
text: contentIsMaximize.value
? $t('preferences.tabbar.contextMenu.restoreMaximize')
: $t('preferences.tabbar.contextMenu.maximize'),
}
/**
* @zh_CN 新窗口打开标签页
* @param tab
*/
async openTabInNewWindow(tab: TabDefinition) {
openRouteInNewWindow(tab.fullPath || tab.path);
},
I think it is more reasonable to continue using fullPath in the two places mentioned above.
There was a problem hiding this comment.
@dingdayu Agreed! You make a valid point. Using fullPath in these specific cases is indeed more appropriate because:
- For router navigation (
router.push), we want to preserve the complete URL including query parameters - For opening in new window, we need the full URL to maintain the exact state
✏️ Learnings added
Learnt from: dingdayu
PR: vbenjs/vue-vben-admin#5500
File: packages/stores/src/modules/tabbar.ts:557-559
Timestamp: 2025-02-08T07:05:28.553Z
Learning: In Vue Vben Admin, while tab identification uses `path` to group tabs with same path but different queries, router navigation operations (router.push) and new window operations should use `fullPath` to preserve the complete URL including query parameters. This applies specifically to:
1. Router navigation in tab maximize/restore feature
2. Opening tabs in new window via openTabInNewWindow
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Resolved an issue where tabs could not be closed due to the recent change in the tabber store key.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/@core/ui-kit/tabs-ui/src/components/tabs-chrome/tabs.vue(1 hunks)packages/@core/ui-kit/tabs-ui/src/components/tabs/tabs.vue(1 hunks)packages/effects/layouts/src/basic/tabbar/use-tabbar.ts(2 hunks)packages/stores/src/modules/tabbar.test.ts(1 hunks)packages/stores/src/modules/tabbar.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/effects/layouts/src/basic/tabbar/use-tabbar.ts
- packages/stores/src/modules/tabbar.ts
🔇 Additional comments (2)
packages/@core/ui-kit/tabs-ui/src/components/tabs/tabs.vue (1)
57-57: LGTM! Change aligns with PR objectives.The modification to use
pathas the primary key withfullPathas fallback aligns with the PR's goal of changing the store key fromfullPathtopath. This change helps preserve query parameters while maintaining tab uniqueness.To ensure this change doesn't break existing functionality, verify that:
- Tabs with the same path but different query parameters are handled correctly
- No duplicate tabs are created when switching between routes
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any other components that might be affected by this change rg -A 5 "fullPath.*path|path.*fullPath" --type vueLength of output: 80
🏁 Script executed:
#!/bin/bash # Re-run the search ensuring .vue files are recognized by ripgrep. # This command manually sets the 'vue' file type to *.vue files. rg --type-add 'vue:*.vue' --type vue -A 5 "key: path \|\| fullPath" packages/@core/ui-kit/tabs-ui/src/components/tabs/tabs.vueLength of output: 268
Verification Completed: Tab Key Implementation is Correct
• The snippet in packages/@core/ui-kit/tabs-ui/src/components/tabs/tabs.vue shows that the key is now defined as “key: path || fullPath”, which aligns with the PR’s objective of preferring “path” (to ensure no duplicate tabs for the same base route) while still falling back to “fullPath” when necessary.
• This change guarantees that even if a route’s query parameters differ, the tab’s uniqueness is maintained by the consistent “path” key.
• There’s no evidence found in the code that this change could inadvertently create duplicate tabs when switching between routes.packages/stores/src/modules/tabbar.test.ts (1)
263-263: LGTM! Test improvement using variable.Using the
keyToClosevariable instead of a hardcoded path makes the test more maintainable and ensures consistency between the test setup and the actual value being tested.
| @@ -48,7 +48,7 @@ const tabsView = computed(() => { | |||
| closable: Reflect.has(meta, 'tabClosable') ? !!tabClosable : true, | |||
| fullPath, | |||
| icon: icon as string, | |||
| key: fullPath || path, | |||
| key: path || fullPath, | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Breaking Change: Tab key prioritization changed from fullPath to path.
This change aligns with the PR objectives to use path as the store key. However, as this is a breaking change that affects tab identification, please ensure:
- The change is documented in the changelog
- A migration guide is provided for users
- Users are warned about potential tab state reset
Please verify the documentation by checking:
🏁 Script executed:
#!/bin/bash
# Search for documentation updates in changelog and migration guides
rg -i "changelog|migration|breaking.*change" -g "*.md"Length of output: 1756
Breaking Change Documentation Update Required
The refactor that switches the tab key determination from using “fullPath” to “path” meets the PR objectives. However, the shell search for documentation only yielded generic changelog and changeset references—with no explicit migration notes addressing this breaking change. This could leave users unprepared for potential tab state resets or key duplications.
• Verify that the changelog (and any migration guide) is updated with explicit details about the tab key change and its impact.
• Provide clear migration instructions that alert users to the possibility of tab state resets or duplications, along with any recommended corrective actions.
There was a problem hiding this comment.
Sorry, CHANGELOG is generated by changesets, I don't know how to update changlog.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for changeset configuration
fd -e json -e yaml -e yml "changeset"
# Check for existing changeset files in .changeset directory
fd . ".changeset" -t f
Length of output: 104
🏁 Script executed:
#!/bin/bash
# Check changeset configuration
cat .changeset/config.json
# Check changeset README for any project-specific instructions
cat .changeset/README.md
Length of output: 1038
@dingdayu Thanks for letting me know! Since the project uses changesets, you can document this breaking change by creating a new changeset. Here's how:
- Run the following command in your terminal:
pnpm changeset- When prompted:
- Select all the packages that are affected (
@vben-core/ui-kit,@vben/stores) - Choose "major" as the bump type since this is a breaking change
- Provide a description like this:
- Select all the packages that are affected (
BREAKING CHANGE: Changed tab key prioritization from `fullPath` to `path`
This change affects how tabs are identified in the system. Previously, tabs were primarily identified by their `fullPath`, with `path` as a fallback. Now, `path` is used as the primary identifier with `fullPath` as fallback.
Impact:
- Existing tab states may be reset
- Tab uniqueness behavior has changed
- Applications relying on `fullPath` for tab identification need to be updated
The changeset will be automatically included in the CHANGELOG when you create a release.
(｀･ω･´)ゞ
✏️ Learnings added
Learnt from: dingdayu
PR: vbenjs/vue-vben-admin#5500
File: packages/@core/ui-kit/tabs-ui/src/components/tabs-chrome/tabs.vue:51-51
Timestamp: 2025-02-08T07:36:14.571Z
Learning: The project uses changesets for version management and changelog generation. Breaking changes should be documented by creating a new changeset using `pnpm changeset` command, selecting affected packages, choosing "major" as the bump type, and providing a detailed description of the change and its impact.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
@mynetfan Any comments on this PR? |
Description
This update introduces breaking changes, so please review carefully.
Changes:
fullPathtopathto improve tabber behavior and resolve #5434.route.querywatch to tabber to prevent loss of URL query parameters when switching tabbers, fixing #5427.Reason:
Using
paramandqueryallows for more flexible tabber uniqueness:query.param.Impact:
Implementation Example:
With this update, table parameters can be passed via URL query, preventing data loss when switching tabbers.
These changes improve tabber navigation consistency and ensure that table filters persist when switching tabs.
Type of change
Please delete options that are not relevant.
Checklist
pnpm run docs:devcommand.pnpm test.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit
Summary by CodeRabbit
New Features
• Enhanced tab management now updates in response to changes in both the URL path and query parameters, resulting in a more dynamic navigation experience.
Bug Fixes
• Improved tab navigation consistency by streamlining how tab paths are determined, ensuring smoother and more accurate tab displays.
• Adjusted key assignment logic for tabs to prioritize path over fullPath, enhancing unique identification within components.