mirror of
https://github.com/multica-ai/multica.git
synced 2026-07-05 13:29:44 +02:00
review: stable selectors + defensive guards on tab-store
Addresses self-review findings on #1239. **C1 — perf cliff from unstable selector returns.** The previous `useActiveTab()` selector used `.find()` inside, so every router tick on the active tab (which replaces the Tab object via immutable spread in updateTab / updateTabHistory) forced every subscriber to re-render. Replaced with finer-grained selectors: - `useActiveTabIdentity()` — { slug, tabId } primitives (stable across unrelated updates). - `useActiveTabRouter()` — stable object reference for a tab's lifetime. - `useActiveTabHistory()` — { historyIndex, historyLength } numbers. `useTabHistory` and `DesktopNavigationProvider` now consume the primitive selectors, so back/forward buttons don't churn on every path change. A non-hook `getActiveTab(state)` helper covers the event-handler case. **I1 — `switchWorkspace` no-ops on empty slug.** Defensive guard in case a malformed path ever reaches the adapter's detector. **I2 — merge warns on path/slug mismatch.** Previously silent drop; now `console.warn` makes the condition visible during debugging. **Misc — TabRouterInner takes `tab` prop directly.** Passing the Tab object eliminates a redundant store read per rendered tab. Known follow-up (not this PR): `packages/core/realtime/use-realtime-sync.ts` still uses `window.location.assign` for workspace-deleted eviction — that's a full renderer reload on desktop, which post-refactor wastes the careful in-memory tab state we just set up. Fixing cleanly requires a navigation-callback injection pattern through CoreProvider, which is cross-cutting and deserves its own PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -1,17 +1,17 @@
|
||||
import { Activity, useEffect } from "react";
|
||||
import { RouterProvider } from "react-router-dom";
|
||||
import { useTabStore, useActiveGroup } from "@/stores/tab-store";
|
||||
import { useActiveGroup } from "@/stores/tab-store";
|
||||
import { TabNavigationProvider } from "@/platform/navigation";
|
||||
import { useTabRouterSync } from "@/hooks/use-tab-router-sync";
|
||||
import type { Tab } from "@/stores/tab-store";
|
||||
|
||||
/** Inner wrapper rendered inside each tab's RouterProvider. */
|
||||
function TabRouterInner({ tabId }: { tabId: string }) {
|
||||
const tab = useTabStore((s) => {
|
||||
if (!s.activeWorkspaceSlug) return null;
|
||||
const group = s.byWorkspace[s.activeWorkspaceSlug];
|
||||
return group?.tabs.find((t) => t.id === tabId) ?? null;
|
||||
});
|
||||
useTabRouterSync(tabId, tab!.router);
|
||||
/**
|
||||
* Inner wrapper rendered inside each tab's RouterProvider. The router
|
||||
* reference is stable for a tab's lifetime, so passing it in directly
|
||||
* (instead of re-deriving from the store) avoids needless re-renders.
|
||||
*/
|
||||
function TabRouterInner({ tab }: { tab: Tab }) {
|
||||
useTabRouterSync(tab.id, tab.router);
|
||||
return null;
|
||||
}
|
||||
|
||||
@@ -46,7 +46,7 @@ export function TabContent() {
|
||||
>
|
||||
<TabNavigationProvider router={tab.router}>
|
||||
<RouterProvider router={tab.router} />
|
||||
<TabRouterInner tabId={tab.id} />
|
||||
<TabRouterInner tab={tab} />
|
||||
</TabNavigationProvider>
|
||||
</Activity>
|
||||
))}
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
import { useCallback } from "react";
|
||||
import type { DataRouter } from "react-router-dom";
|
||||
import { useActiveTab } from "@/stores/tab-store";
|
||||
import { useActiveTabRouter, useActiveTabHistory } from "@/stores/tab-store";
|
||||
|
||||
/**
|
||||
* Shared hint map so useTabRouterSync can distinguish back vs forward POP.
|
||||
@@ -10,28 +10,31 @@ export const popDirectionHints = new Map<DataRouter, "back" | "forward">();
|
||||
|
||||
/**
|
||||
* Per-tab back/forward navigation derived from the active workspace's
|
||||
* active tab. When the active workspace has no tab (pre-workspace state),
|
||||
* both buttons disable gracefully.
|
||||
* active tab.
|
||||
*
|
||||
* Subscribed via primitive selectors so this hook only re-renders when
|
||||
* the numeric history state actually changes — path ticks on the active
|
||||
* tab (which don't shift historyIndex) don't churn the back/forward
|
||||
* buttons.
|
||||
*/
|
||||
export function useTabHistory() {
|
||||
const activeTab = useActiveTab();
|
||||
const router = useActiveTabRouter();
|
||||
const { historyIndex, historyLength } = useActiveTabHistory();
|
||||
|
||||
const canGoBack = (activeTab?.historyIndex ?? 0) > 0;
|
||||
const canGoForward =
|
||||
(activeTab?.historyIndex ?? 0) < (activeTab?.historyLength ?? 1) - 1;
|
||||
const canGoBack = historyIndex > 0;
|
||||
const canGoForward = historyIndex < historyLength - 1;
|
||||
|
||||
const goBack = useCallback(() => {
|
||||
if (!activeTab || activeTab.historyIndex <= 0) return;
|
||||
popDirectionHints.set(activeTab.router, "back");
|
||||
activeTab.router.navigate(-1);
|
||||
}, [activeTab]);
|
||||
if (!router || historyIndex <= 0) return;
|
||||
popDirectionHints.set(router, "back");
|
||||
router.navigate(-1);
|
||||
}, [router, historyIndex]);
|
||||
|
||||
const goForward = useCallback(() => {
|
||||
if (!activeTab || activeTab.historyIndex >= activeTab.historyLength - 1)
|
||||
return;
|
||||
popDirectionHints.set(activeTab.router, "forward");
|
||||
activeTab.router.navigate(1);
|
||||
}, [activeTab]);
|
||||
if (!router || historyIndex >= historyLength - 1) return;
|
||||
popDirectionHints.set(router, "forward");
|
||||
router.navigate(1);
|
||||
}, [router, historyIndex, historyLength]);
|
||||
|
||||
return { canGoBack, canGoForward, goBack, goForward };
|
||||
}
|
||||
|
||||
@@ -6,7 +6,13 @@ import {
|
||||
} from "@multica/views/navigation";
|
||||
import { useAuthStore } from "@multica/core/auth";
|
||||
import { isReservedSlug } from "@multica/core/paths";
|
||||
import { useTabStore, resolveRouteIcon } from "@/stores/tab-store";
|
||||
import {
|
||||
useTabStore,
|
||||
resolveRouteIcon,
|
||||
useActiveTabIdentity,
|
||||
useActiveTabRouter,
|
||||
getActiveTab,
|
||||
} from "@/stores/tab-store";
|
||||
import { useWindowOverlayStore } from "@/stores/window-overlay-store";
|
||||
|
||||
// Public web app URL — injected at build time via .env.production. Falls
|
||||
@@ -95,25 +101,25 @@ export function DesktopNavigationProvider({
|
||||
}: {
|
||||
children: React.ReactNode;
|
||||
}) {
|
||||
const activeTab = useTabStore((s) => {
|
||||
if (!s.activeWorkspaceSlug) return null;
|
||||
const group = s.byWorkspace[s.activeWorkspaceSlug];
|
||||
if (!group) return null;
|
||||
return group.tabs.find((t) => t.id === group.activeTabId) ?? null;
|
||||
});
|
||||
const [pathname, setPathname] = useState(activeTab?.path ?? "/");
|
||||
// Primitive-only subscriptions so this component doesn't re-render on
|
||||
// unrelated store updates (e.g. an inactive tab's router tick). We
|
||||
// resolve the active router here only to subscribe once per tab switch.
|
||||
const { tabId: activeTabId } = useActiveTabIdentity();
|
||||
const router = useActiveTabRouter();
|
||||
const [pathname, setPathname] = useState(
|
||||
router?.state.location.pathname ?? "/",
|
||||
);
|
||||
|
||||
// Subscribe to the active tab's router for pathname updates.
|
||||
useEffect(() => {
|
||||
if (!activeTab) {
|
||||
if (!router) {
|
||||
setPathname("/");
|
||||
return;
|
||||
}
|
||||
setPathname(activeTab.router.state.location.pathname);
|
||||
return activeTab.router.subscribe((state) => {
|
||||
setPathname(router.state.location.pathname);
|
||||
return router.subscribe((state) => {
|
||||
setPathname(state.location.pathname);
|
||||
});
|
||||
}, [activeTab?.id]); // eslint-disable-line react-hooks/exhaustive-deps
|
||||
}, [activeTabId, router]);
|
||||
|
||||
const adapter: NavigationAdapter = useMemo(
|
||||
() => ({
|
||||
@@ -160,11 +166,7 @@ export function DesktopNavigationProvider({
|
||||
}
|
||||
|
||||
function currentActiveTab() {
|
||||
const state = useTabStore.getState();
|
||||
if (!state.activeWorkspaceSlug) return null;
|
||||
const group = state.byWorkspace[state.activeWorkspaceSlug];
|
||||
if (!group) return null;
|
||||
return group.tabs.find((t) => t.id === group.activeTabId) ?? null;
|
||||
return getActiveTab(useTabStore.getState());
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -230,6 +230,10 @@ export const useTabStore = create<TabStore>()(
|
||||
byWorkspace: {},
|
||||
|
||||
switchWorkspace(slug, openPath) {
|
||||
// Defensive no-op if slug is empty/invalid — callers like the
|
||||
// NavigationAdapter's path-parser should already have filtered
|
||||
// these, but belt-and-braces keeps garbage out of the store.
|
||||
if (!slug) return;
|
||||
const { byWorkspace } = get();
|
||||
const existing = byWorkspace[slug];
|
||||
|
||||
@@ -514,7 +518,14 @@ export const useTabStore = create<TabStore>()(
|
||||
// Persisted path may have come from a stale version or a
|
||||
// manual edit. Drop rather than rewrite so we never silently
|
||||
// put users on a path that doesn't match the group's slug.
|
||||
if (!clean || extractWorkspaceSlug(clean) !== slug) continue;
|
||||
if (!clean || extractWorkspaceSlug(clean) !== slug) {
|
||||
// eslint-disable-next-line no-console
|
||||
console.warn(
|
||||
`[tab-store] dropping persisted tab "${pTab.path}" from ` +
|
||||
`group "${slug}" — path/slug mismatch`,
|
||||
);
|
||||
continue;
|
||||
}
|
||||
tabs.push({
|
||||
id: pTab.id,
|
||||
path: clean,
|
||||
@@ -622,19 +633,73 @@ export function migrateV1ToV2(v1: Partial<V1Persisted>): V2Persisted {
|
||||
// Selectors (convenience hooks)
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/** The active workspace's tab group, or null when no workspace is active. */
|
||||
/**
|
||||
* Pure non-hook helper — useful from event handlers / effects that already
|
||||
* need `.getState()`. For React subscriptions prefer the stable selectors
|
||||
* below.
|
||||
*/
|
||||
export function getActiveTab(s: TabStore): Tab | null {
|
||||
if (!s.activeWorkspaceSlug) return null;
|
||||
const group = s.byWorkspace[s.activeWorkspaceSlug];
|
||||
if (!group) return null;
|
||||
return group.tabs.find((t) => t.id === group.activeTabId) ?? null;
|
||||
}
|
||||
|
||||
/**
|
||||
* The active workspace's tab group, or null when no workspace is active.
|
||||
*
|
||||
* Zustand compares selector returns with `Object.is`. Because `updateTab`
|
||||
* / `updateTabHistory` replace the group object on every router tick
|
||||
* (immutable update), this selector returns a new reference on every
|
||||
* router event — that's fine for TabBar which needs to observe tab-list
|
||||
* changes, but don't use this selector from components that only care
|
||||
* about one primitive (use `useActiveTabHistory` / `useActiveTabRouter`
|
||||
* instead).
|
||||
*/
|
||||
export function useActiveGroup(): WorkspaceTabGroup | null {
|
||||
return useTabStore((s) =>
|
||||
s.activeWorkspaceSlug ? (s.byWorkspace[s.activeWorkspaceSlug] ?? null) : null,
|
||||
);
|
||||
}
|
||||
|
||||
/** The currently rendered tab, or null. */
|
||||
export function useActiveTab(): Tab | null {
|
||||
return useTabStore((s) => {
|
||||
if (!s.activeWorkspaceSlug) return null;
|
||||
const group = s.byWorkspace[s.activeWorkspaceSlug];
|
||||
if (!group) return null;
|
||||
return group.tabs.find((t) => t.id === group.activeTabId) ?? null;
|
||||
});
|
||||
/**
|
||||
* Active tab id + active workspace slug as a compact pair. Both primitives
|
||||
* are stable across unrelated store updates — e.g. an inactive tab's
|
||||
* router tick doesn't churn these, so consumers don't re-render.
|
||||
*
|
||||
* Useful anywhere you'd previously have reached for `useActiveTab()` and
|
||||
* only needed the identity (for memoization, effect deps, ipc).
|
||||
*/
|
||||
export function useActiveTabIdentity(): { slug: string | null; tabId: string | null } {
|
||||
const slug = useTabStore((s) => s.activeWorkspaceSlug);
|
||||
const tabId = useTabStore((s) =>
|
||||
s.activeWorkspaceSlug
|
||||
? (s.byWorkspace[s.activeWorkspaceSlug]?.activeTabId ?? null)
|
||||
: null,
|
||||
);
|
||||
return { slug, tabId };
|
||||
}
|
||||
|
||||
/**
|
||||
* Active tab's router — a stable reference across tab updates, because
|
||||
* routers are created once per tab and never replaced by `updateTab`.
|
||||
* Subscribers only re-render when the active tab *changes*, not on
|
||||
* router events within the current tab.
|
||||
*/
|
||||
export function useActiveTabRouter(): DataRouter | null {
|
||||
return useTabStore((s) => getActiveTab(s)?.router ?? null);
|
||||
}
|
||||
|
||||
/**
|
||||
* History tracking for the active tab as primitives. Subscribers re-render
|
||||
* only when the numeric index / length change (i.e. on actual navigations),
|
||||
* not on unrelated store updates.
|
||||
*/
|
||||
export function useActiveTabHistory(): {
|
||||
historyIndex: number;
|
||||
historyLength: number;
|
||||
} {
|
||||
const historyIndex = useTabStore((s) => getActiveTab(s)?.historyIndex ?? 0);
|
||||
const historyLength = useTabStore((s) => getActiveTab(s)?.historyLength ?? 1);
|
||||
return { historyIndex, historyLength };
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user