Skip to content

fix(navigation): preserve redirecting pages and descendants #660

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
IzumiSy wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix(navigation): preserve redirecting pages and descendants #660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/tidy-guards-navigate.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@tailor-platform/app-shell": patch
---

Keep redirecting pages and their visible descendants in automatic sidebar navigation and Command Palette entries, including current-path-aware dynamic routes.
Include redirect-only pages as links to their original paths even without a component. Hidden guards continue to exclude their subtrees.
2 changes: 1 addition & 1 deletion packages/core/src/components/command-palette/command-palette.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export function navItemsToRoutes(navItems: Array<NavItem>): Array<NavigatableRou
items.forEach((item) => {
const breadcrumb = [...parentBreadcrumb, item.title];

// Only add navigable routes (componentless resources have no url)
// Only add navigable routes
if (item.url) {
routes.push({
path: item.url,
Expand Down
60 changes: 59 additions & 1 deletion packages/core/src/components/sidebar/default-sidebar.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { SidebarItem } from "./sidebar-item";
import { SidebarGroup } from "./sidebar-group";
import { SidebarSection } from "./sidebar-section";
import { SidebarSeparator } from "./sidebar-separator";
import { defineModule, defineResource } from "@/resource";
import { defineModule, defineResource, redirectTo } from "@/resource";
import { AppShell } from "@/components/appshell";
import { useURLCollectionVariables } from "@/lib/collection-url-state";
import { useEffect } from "react";
Expand Down Expand Up @@ -169,6 +169,64 @@ describe("DefaultSidebar", () => {
expect(screen.getByRole("link", { name: /products/i })).toBeDefined();
});

it.each([true, false])(
"keeps redirecting parents in auto navigation and their descendants in the palette (has component: %s)",
async (hasComponent) => {
const user = userEvent.setup();
stubDesktopViewport();
const modules = [
defineModule({
path: "workspace",
meta: { title: "Workspace" },
component: hasComponent ? () => <div>Workspace Root</div> : undefined,
guards: [() => redirectTo("/workspace/section/list")],
resources: [
defineResource({
path: "section",
meta: { title: "Section" },
component: hasComponent ? () => <div>Section Root</div> : undefined,
guards: [() => redirectTo("/workspace/section/list")],
subResources: [
defineResource({
path: "list",
meta: { title: "List" },
component: () => <div>List Page</div>,
}),
],
}),
],
}),
];

window.history.pushState({}, "", "/workspace/section/list");
render(
<AppShell title="Test" modules={modules}>
<SidebarLayout />
</AppShell>,
);

expect(await screen.findByText("List Page")).toBeDefined();
const sidebar = document.querySelector<HTMLElement>('[data-slot="sidebar"]');
assert(sidebar);
const moduleLink = await within(sidebar).findByRole("link", { name: "Workspace" });
expect(moduleLink.getAttribute("href")).toBe("/workspace");
const resourceLink = await within(sidebar).findByRole("link", { name: "Section" });
expect(resourceLink.getAttribute("href")).toBe("/workspace/section");

await user.click(within(sidebar).getByRole("button", { name: /search/i }));

const dialog = await screen.findByRole("dialog");
const descendantEntry = await within(dialog).findByRole("button", {
name: "Workspace > Section > List /workspace/section/list",
});
expect(descendantEntry).toBeDefined();
expect(within(dialog).getByRole("button", { name: "Workspace /workspace" })).toBeDefined();
expect(
within(dialog).getByRole("button", { name: "Workspace > Section /workspace/section" }),
).toBeDefined();
},
);

it("keeps explicit sidebar navigation working after a page writes search params", async () => {
// Asserts on the link's text label, so the sidebar must be expanded rather
// than an icon rail.
Expand Down
200 changes: 199 additions & 1 deletion packages/core/src/routing/navigation.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { useMemo } from "react";
import { describe, it, expect, afterEach, vi } from "vitest";
import { useNavItems, buildCurrentPathAwareRoutes } from "./navigation";
import { useLocation, useNavigate } from "react-router";
import { defineModule, defineResource, hidden, pass } from "@/resource";
import { defineModule, defineResource, hidden, pass, redirectTo } from "@/resource";
import {
AppShellConfigContext,
AppShellDataContext,
Expand Down Expand Up @@ -99,6 +99,112 @@ describe("useNavItems", () => {
});
});

it.each([true, false])(
"keeps redirecting parents and visible descendants (has component: %s)",
async (hasComponent) => {
const component = hasComponent ? () => <div>Parent</div> : undefined;
const modules = [
defineModule({
path: "workspace",
meta: { title: "Workspace" },
component,
guards: [() => redirectTo("/workspace/section/list")],
resources: [
defineResource({
path: "section",
component,
guards: [() => redirectTo("/workspace/section/list")],
subResources: [
defineResource({
path: "list",
component: () => <div>List</div>,
}),
defineResource({
path: "secret",
guards: [() => hidden()],
subResources: [
defineResource({
path: "details",
component: () => <div>Secret Details</div>,
}),
],
}),
],
}),
],
}),
];

const { result } = renderNavItems(modules, "/workspace/section/list");

await waitFor(async () => {
expect(await result.current!).toEqual([
{
title: "Workspace",
url: "workspace",
icon: expect.anything(),
items: [
{
title: "Section",
url: "workspace/section",
items: [{ title: "List", url: "workspace/section/list" }],
},
],
},
]);
});
},
);

it.each([
{ result: "pass", guard: pass },
{ result: "redirect", guard: () => redirectTo("/app") },
{ result: "hidden", guard: hidden },
])(
"handles componentless leaf navigation when guards return $result",
async ({ result, guard }) => {
const modules = [
defineModule({
path: "",
meta: { title: "Home" },
guards: [guard],
resources: [],
}),
defineModule({
path: "legacy",
meta: { title: "Legacy" },
guards: [guard],
resources: [],
}),
defineModule({
path: "app",
meta: { title: "App" },
component: () => <div>App</div>,
resources: [defineResource({ path: "alias", guards: [guard] })],
}),
];

const { result: navItems } = renderNavItems(modules, "/app");

await waitFor(async () => {
expect(await navItems.current!).toEqual([
...(result === "redirect"
? [
{ title: "Home", url: "/", icon: expect.anything(), items: [] },
{ title: "Legacy", url: "legacy", icon: expect.anything(), items: [] },
]
: []),
{
title: "App",
url: "app",
icon: expect.anything(),
items: result === "redirect" ? [{ title: "Alias", url: "app/alias" }] : [],
},
]);
});
},
);

it("filters out modules hidden by guards", async () => {
const modules = [
defineModule({
Expand Down Expand Up @@ -650,6 +756,98 @@ describe("useCommandPaletteRoutes", () => {
});
});

it.each([true, false])(
"keeps descendants of redirecting dynamic and static parents (has component: %s)",
async (hasComponent) => {
const component = hasComponent ? () => <div>Parent</div> : undefined;
const modules = [
defineModule({
path: "users",
meta: { title: "Users" },
resources: [
defineResource({
path: ":id",
component,
guards: [() => redirectTo("/users/42/section/list")],
subResources: [
defineResource({
path: "section",
component,
guards: [() => redirectTo("/users/42/section/list")],
subResources: [
defineResource({
path: "list",
component: () => <div>List</div>,
}),
defineResource({
path: "secret",
guards: [() => hidden()],
subResources: [
defineResource({
path: "details",
component: () => <div>Secret Details</div>,
}),
],
}),
],
}),
],
}),
],
}),
];

const { result } = renderWithNavigationLoader(
() => useCommandPaletteRoutes(),
modules,
"/users/42/section/list",
);

await waitFor(async () => {
const routes = await result.current!;
expect(routes.map(({ path }) => path)).toEqual([
"users/42",
"users/42/section",
"users/42/section/list",
]);
});
},
);

it.each([
{ result: "pass", guard: pass },
{ result: "redirect", guard: () => redirectTo("/users/42/profile") },
{ result: "hidden", guard: hidden },
])(
"handles a componentless dynamic module when guards return $result",
async ({ result, guard }) => {
const modules = [
defineModule({
path: "users/:id",
meta: { title: "User" },
guards: [guard],
resources: [
defineResource({ path: "profile", component: () => <div>Profile</div> }),
defineResource({ path: "alias", guards: [() => redirectTo("/users/42/profile")] }),
defineResource({ path: "namespace", guards: [pass] }),
],
}),
];

const routes = await buildCurrentPathAwareRoutes({
modules,
locale: "en",
pathname: "/users/42/profile",
});

expect(routes.map(({ path }) => path)).toEqual(
result === "hidden"
? []
: [...(result === "redirect" ? ["users/42"] : []), "users/42/profile", "users/42/alias"],
);
},
);

it("keeps traversing through a non-navigable dynamic namespace", async () => {
const modules = [
defineModule({
Expand Down
26 changes: 14 additions & 12 deletions packages/core/src/routing/navigation.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,26 +68,27 @@ const buildNavItems = async (props: BuildNavItemsProps) => {
if (parseDynamicSegment(module.path) !== null) return null;

const guardResult = await runGuards(module.guards);
if (guardResult.type !== "pass") return null;
if (guardResult.type === "hidden") return null;

const visibleResources = await filterVisibleResources(
module.resources,
module.path,
resolveTitle,
);
const isNavigable = module.meta.menuItemClickable || guardResult.type === "redirect";
// Skip if no visible resources AND the module itself is not directly navigable
if (visibleResources.length === 0 && !module.meta.menuItemClickable) return null;
if (visibleResources.length === 0 && !isNavigable) return null;

return { module, resources: visibleResources };
return { module, resources: visibleResources, isNavigable };
}),
);

return resolvedModules
.filter((entry) => entry !== null)
.map(({ module, resources }) => {
.map(({ module, resources, isNavigable }) => {
return {
title: resolveTitle(module.meta.title, module.path),
url: module.meta.menuItemClickable ? module.path || "/" : undefined,
url: isNavigable ? module.path || "/" : undefined,
icon: module.meta.icon || <Table />,
items: resources,
};
Expand All @@ -105,11 +106,11 @@ const filterVisibleResources = async (
if (parseDynamicSegment(resource.path) !== null) return null;

const guardResult = await runGuards(resource.guards);
if (guardResult.type !== "pass") return null;
if (guardResult.type === "hidden") return null;

const resourcePath = `${basePath}/${resource.path}`;
const resourceTitle = resolveTitle(resource.meta.title, resource.path);
const hasComponent = resource.component !== undefined;
const isNavigable = resource.component !== undefined || guardResult.type === "redirect";

// Recursively process subResources
const subItems = resource.subResources
Expand All @@ -118,12 +119,12 @@ const filterVisibleResources = async (

const hasVisibleSubItems = subItems && subItems.length > 0;

// Componentless resources without sub-resources are dead-end namespaces
if (!hasComponent && !hasVisibleSubItems) return null;
// Non-navigable resources without sub-resources are dead-end namespaces
if (!isNavigable && !hasVisibleSubItems) return null;

return {
title: resourceTitle,
url: hasComponent ? resourcePath : undefined,
url: isNavigable ? resourcePath : undefined,
items: hasVisibleSubItems ? subItems : undefined,
};
}),
Expand Down Expand Up @@ -251,10 +252,11 @@ const collectCurrentPathAwareRoutes = async ({
routeMap: Map<string, NavigatableRoute>;
}): Promise<void> => {
const guardResult = await runGuards(node.guards);
if (guardResult.type !== "pass") return;
if (guardResult.type === "hidden") return;

const isNavigable =
"resources" in node ? node.meta.menuItemClickable : node.component !== undefined;
guardResult.type === "redirect" ||
("resources" in node ? node.meta.menuItemClickable : node.component !== undefined);
if (isNavigable) {
const path = routePathFromSegments(baseSegments);
routeMap.set(path, {
Expand Down
Loading