Repository navigation
perf: optimize the closing jump logic of tabs - #7518
Conversation
* 依据tab访问历史回退上一个tab，原逻辑是返回一下个 或 上一个
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the
📝 WalkthroughWalkthroughThis PR adds a generic Stack implementation (with dedup and maxSize), test coverage for it, and integrates a visit-history Stack into the tabbar store plus preference/UI wiring to enable/disable visit history. Changes
Sequence DiagramsequenceDiagram
actor User
participant TabComponent as Tab Component
participant TabbarStore as Tabbar Store
participant VisitHistory as Visit History (Stack)
participant SessionStorage as Session Storage
User->>TabComponent: Click tab / Select tab
TabComponent->>TabbarStore: addTab / selectTab(key)
TabbarStore->>VisitHistory: push(key)
VisitHistory->>VisitHistory: dedup + enforce maxSize
TabbarStore->>SessionStorage: persist visitHistory
User->>TabComponent: Close tab (active)
TabComponent->>TabbarStore: closeTab(key)
TabbarStore->>VisitHistory: remove(key)
TabbarStore->>VisitHistory: peek()
alt previous tab exists
TabbarStore->>TabComponent: navigate to previous tab
else none
TabbarStore->>TabComponent: fallback navigation (default/next)
end
TabbarStore->>SessionStorage: persist visitHistory
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/stores/src/modules/tabbar.ts (1)
135-158:⚠️ Potential issue | 🟠 MajorKeep visitHistory in sync when auto-evicting tabs.
When maxNumOfOpenTab/maxCount triggers a splice, the evicted tab key remains in history and can be popped later, leading to navigation to a closed tab. Remove the evicted key at the same time.🛠️ Suggested fix
- index !== -1 && this.tabs.splice(index, 1); + if (index !== -1) { + const [removed] = this.tabs.splice(index, 1); + if (removed) { + this.visitHistory.remove(getTabKeyFromTab(removed)); + } + } @@ - index !== -1 && this.tabs.splice(index, 1); + if (index !== -1) { + const [removed] = this.tabs.splice(index, 1); + if (removed) { + this.visitHistory.remove(getTabKeyFromTab(removed)); + } + }
🤖 Fix all issues with AI agents
In `@packages/stores/src/modules/tabbar.ts`:
- Around line 281-288: After closing a tab (you call
this.visitHistory.remove(currentTabKey) and this._close(tab)), guard against
stale keys returned by this.visitHistory.pop() by looping: repeatedly pop keys
and call this.getTabByKey(previousTabKey) until you find a non-undefined tab or
visitHistory is exhausted; if no valid tab is found, fall back to a safe default
tab (e.g., the first open tab or a home tab) and call await
this._goToTab(validTab, router). Ensure you don’t call this._goToTab with
undefined and keep using the existing methods _close, visitHistory.pop(),
getTabByKey, and _goToTab.
- Around line 540-548: Change the persist hook from afterRestore to afterHydrate
and, inside that afterHydrate handler for the tabbar store, detect when
visitHistory was deserialized and rehydrate it to a proper Stack instance: use
createStack(true, MAX_VISIT_HISTORY) to create a new stack, populate it with the
deserialized items (preserving order) and assign it back to visitHistory on the
store (TabbarState.visitHistory), so the restored object regains its methods
(push/remove/pop/retain); ensure this hook runs only for the persisted keys
(pick includes 'visitHistory') and replaces the plain object with the new Stack
instance.
| persist: [ | ||
| // tabs不需要保存在localStorage | ||
| { | ||
| pick: ['tabs'], | ||
| pick: ['tabs', 'visitHistory'], | ||
| storage: sessionStorage, | ||
| }, | ||
| ], | ||
| state: (): TabbarState => ({ | ||
| visitHistory: createStack<string>(true, MAX_VISIT_HISTORY), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the tabbar.ts file and examine the full persist configuration
fd -t f "tabbar.ts" | head -5Repository: vbenjs/vue-vben-admin
Length of output: 158
🏁 Script executed:
#!/bin/bash
# Search for Stack implementation
rg -n "createStack|class Stack|export.*Stack" -A 3Repository: vbenjs/vue-vben-admin
Length of output: 2233
🏁 Script executed:
#!/bin/bash
# Look for persistence plugin configuration and any existing hooks
rg -n "pinia-plugin-persistedstate|createPersistedState|afterRestore|beforeRestore" -A 3 -B 1Repository: vbenjs/vue-vben-admin
Length of output: 2542
🏁 Script executed:
#!/bin/bash
# Search for how visitHistory is used in the codebase to understand method calls
rg -n "visitHistory\." | head -20Repository: vbenjs/vue-vben-admin
Length of output: 644
🏁 Script executed:
#!/bin/bash
# Read the full persist configuration in tabbar.ts
sed -n '535,560p' packages/stores/src/modules/tabbar.tsRepository: vbenjs/vue-vben-admin
Length of output: 721
🏁 Script executed:
#!/bin/bash
# Read the Stack class implementation to understand its structure
cat -n packages/@core/base/shared/src/utils/stack.tsRepository: vbenjs/vue-vben-admin
Length of output: 2573
🏁 Script executed:
#!/bin/bash
# Check if there's already a restore hook or serializer in the persistence setup
rg -n "afterRestore|beforeRestore|serializer|merge" packages/stores/src/modules/tabbar.ts -B 2 -A 5Repository: vbenjs/vue-vben-admin
Length of output: 905
🏁 Script executed:
#!/bin/bash
# Find MAX_VISIT_HISTORY constant
rg -n "MAX_VISIT_HISTORY" packages/stores/src/modules/tabbar.ts -B 2 -A 2Repository: vbenjs/vue-vben-admin
Length of output: 310
🌐 Web query:
pinia-plugin-persistedstate v4.7.1 afterRestore onHydrate hook signature documentation
💡 Result:
In pinia-plugin-persistedstate v4.7.1, the old v2 hook names beforeRestore / afterRestore were renamed to beforeHydrate / afterHydrate (and the global beforeRestore/afterRestore options were dropped in v4). [3]
v4.7.1 hook signatures (docs)
beforeHydrate?: (context: PiniaPluginContext) => void[1]afterHydrate?: (context: PiniaPluginContext) => void[1]
So if you’re looking for afterRestore, the v4 equivalent is afterHydrate(ctx). [1][3]
“onHydrate”?
There isn’t an onHydrate hook in the v4 docs; hydration is done by the plugin, and you can also trigger it manually via:
store.$hydrate({ runHooks: false })(by default it runsbeforeHydrate/afterHydrate). [2]
Where afterRestore is documented (older docs)
If you specifically need the afterRestore signature, that’s documented in the v2 docs as:
afterRestore?: (context: PiniaPluginContext) => void[4]
Sources: [1] v4 config docs (beforeHydrate/afterHydrate types), [2] v4 advanced docs ($hydrate and runHooks), [3] v4.0.0 release notes (rename before/afterRestore → before/afterHydrate), [4] v2 config docs (afterRestore type).
Fix hook name: afterRestore → afterHydrate for pinia-plugin-persistedstate v4.7.1.
Persisting Stack directly will break after restore. Pinia's persistence serializes to JSON, so visitHistory deserializes as a plain object and loses methods (push/remove/pop/retain), causing runtime crashes. Rehydrate to a Stack instance in an afterHydrate hook.
🛠️ Suggested fix
persist: [
// tabs不需要保存在localStorage
{
pick: ['tabs', 'visitHistory'],
storage: sessionStorage,
+ afterHydrate: (ctx) => {
+ const raw = ctx.store.visitHistory as any;
+ const history = createStack<string>(true, MAX_VISIT_HISTORY);
+ const items = Array.isArray(raw) ? raw : raw?.items ?? raw?.toArray?.() ?? [];
+ history.push(...items);
+ ctx.store.visitHistory = history;
+ },
},
],🤖 Prompt for AI Agents
In `@packages/stores/src/modules/tabbar.ts` around lines 540 - 548, Change the
persist hook from afterRestore to afterHydrate and, inside that afterHydrate
handler for the tabbar store, detect when visitHistory was deserialized and
rehydrate it to a proper Stack instance: use createStack(true,
MAX_VISIT_HISTORY) to create a new stack, populate it with the deserialized
items (preserving order) and assign it back to visitHistory on the store
(TabbarState.visitHistory), so the restored object regains its methods
(push/remove/pop/retain); ensure this hook runs only for the persisted keys
(pick includes 'visitHistory') and replaces the plain object with the new Stack
instance.
|
关闭逻辑增加一个选项 可以定义用哪种模式 |
* 依据tab访问历史回退上一个tab，原逻辑是返回一下个 或 上一个 * 支持在配置中开启或关闭
* 依据tab访问历史回退上一个tab，原逻辑是返回一下个 或 上一个 * 支持在配置中开启或关闭
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/stores/src/modules/tabbar.ts`:
- Around line 183-186: The addTab call currently appends every tab to
visitHistory (via this.visitHistory.push) which causes affix tabs added by
setAffixTabs at startup to be recorded as user visits; change addTab (or the
call site in setAffixTabs) to skip pushing to visitHistory for affix tabs by
checking tab.affix (or add an optional parameter like skipHistory) and only call
this.visitHistory.push when isVisitHistory() is true AND the tab is not an affix
tab.
🧹 Nitpick comments (1)
packages/stores/src/modules/tabbar.ts (1)
686-691:isVisitHistory()reads a reactive preference outside of a reactive context.This helper accesses
preferences.tabbar.visitHistorydirectly (not viacomputedor inside a Vue reactive scope). Within Pinia actions this is fine since it's called imperatively, but be aware this won't track reactively if ever used in a getter or computed property.
| // 添加访问历史记录 | ||
| if (isVisitHistory()) { | ||
| this.visitHistory.push(tab.key as string); | ||
| } |
There was a problem hiding this comment.
addTab is called for affix tabs during setAffixTabs — history records affix tabs too.
When setAffixTabs calls addTab for each affix tab at startup, those tabs get pushed into visitHistory. This means the visit history will contain affix tab entries from initialization, which may not reflect actual user visits. Consider whether this is intentional — if not, you could skip the history push for affix tabs.
🤖 Prompt for AI Agents
In `@packages/stores/src/modules/tabbar.ts` around lines 183 - 186, The addTab
call currently appends every tab to visitHistory (via this.visitHistory.push)
which causes affix tabs added by setAffixTabs at startup to be recorded as user
visits; change addTab (or the call site in setAffixTabs) to skip pushing to
visitHistory for affix tabs by checking tab.affix (or add an optional parameter
like skipHistory) and only call this.visitHistory.push when isVisitHistory() is
true AND the tab is not an affix tab.
* 依据tab访问历史回退上一个tab，原逻辑是返回一下个 或 上一个 * 支持在配置中开启或关闭
Summary by CodeRabbit
New Features
UI
Localization
Tests