Files
multica/apps/web/platform/navigation.test.tsx
Bohan Jiang ceba14a227 fix(issues): MUL-5362 return to the source list after deleting an issue (#5997)
* fix(issues): return to the source list after deleting an issue

Deleting an issue from its detail page always pushed the workspace
Issues list, so opening an issue from My Issues (or a project list,
search, a pin, an agent panel) and deleting it dropped the user's
navigation context — GH #5995.

Go back instead. `useBackOrReplace` steps back when the platform
reports in-app history and replaces with a fallback path when there
is none, so a shared link opened cold or a new tab never steps off
the app. Web answers via the Navigation API, falling back to counting
its own pushes; desktop reads the active tab's virtual history.

`replace`, not `push`: the deleted issue's URL must not stay in
history for the back button to land on a 404.

The not-found "Back to Issues" button loses the same context, so it
moves to the same helper and its label becomes a plain "Back".

Co-authored-by: multica-agent <github@multica.ai>

* fix(web): track history position, not push count, for canGoBack

The Navigation API fallback only ever counted pushes, so `pushes > 0`
did not mean the current entry still had an in-app page behind it.
Cold-open an issue, click Issues, press the browser's Back button, then
delete: the tracker still claimed history and `back()` would step off
Multica — the exact case the fallback exists to prevent.

Count depth instead: a push adds one, any traversal takes one away
(clamped at zero). Reaching the document's first entry requires
traversing back at least as many times as we pushed, so a positive
depth can never be claimed while sitting on it. A browser Forward is
now conservative — it reports no history where a step back would have
been fine — which costs a fallback navigation rather than an exit.

Also corrects the useBackOrReplace contract comment: stepping back
leaves the dead URL in forward history, so the guarantee is that it
never lands on the back stack, not that it leaves history entirely.

Co-authored-by: multica-agent <github@multica.ai>

* fix(web): answer canGoBack from the browser alone, never from push counts

Review found the counter still lied, one level deeper: it counted
`router.push` calls, and a call is not a committed history entry. Next
drops a push to `replaceState` when the canonical URL is unchanged
(app-router.js, the `pendingPush && href !== canonicalUrl` branch), so
clicking a self-link — the breadcrumb on an issue detail page, a pin to
the issue you are on — incremented depth with no entry behind it, and a
delete from there could still step off the app.

Fixing the count needs per-entry markers, which means depending on both
React effect ordering (our provider's effect runs before app-router's
history commit) and Next's own preserveCustomHistoryState behaviour.
Two rounds of review have now found holes in hand-rolled history
tracking; a third layer of it is not the way to buy this guarantee.

So stop deriving. `canGoBack` is the Navigation API's answer or `false`.
Browsers without it take the fallback path, which is exactly what they
did before any of this existed — nobody regresses, and the "wrong true
walks the user out of the app" failure is now unreachable by
construction.

Drops the tracker, the popstate wiring and the push wrapper: the
`multica:navigate` bridge returns to its original shape.

Co-authored-by: multica-agent <github@multica.ai>

---------

Co-authored-by: Bohan-J <bohan@devv.ai>
Co-authored-by: multica-agent <github@multica.ai>
2026-07-27 18:39:25 +08:00

126 lines
3.7 KiB
TypeScript

/**
* MUL-5208 — the web half of the `multica:navigate` bridge.
*
* Shared content (comments, chat, issue descriptions) fires this event whenever
* a link resolves to an in-app destination, including an absolute URL on this
* deployment's own origin. Desktop answers it by opening a tab; the web must
* answer it with a router push, or those links silently do nothing.
*/
import { describe, expect, it, vi, afterEach, beforeEach } from "vitest";
import { render } from "@testing-library/react";
const router = vi.hoisted(() => ({
push: vi.fn(),
replace: vi.fn(),
back: vi.fn(),
prefetch: vi.fn(),
}));
vi.mock("next/navigation", () => ({
useRouter: () => router,
usePathname: () => "/acme/issues",
useSearchParams: () => new URLSearchParams(),
}));
import { WebNavigationProvider } from "./navigation";
import { useNavigation, type NavigationAdapter } from "@multica/views/navigation";
function navigate(path: string) {
window.dispatchEvent(
new CustomEvent("multica:navigate", { detail: { path } }),
);
}
function renderAdapter(): () => NavigationAdapter {
let adapter: NavigationAdapter | null = null;
function Probe() {
adapter = useNavigation();
return null;
}
render(
<WebNavigationProvider>
<Probe />
</WebNavigationProvider>,
);
return () => adapter!;
}
beforeEach(() => {
router.push.mockReset();
});
describe("WebNavigationProvider internal link bridge", () => {
it("pushes the path a content link resolved to", () => {
render(<WebNavigationProvider>{null}</WebNavigationProvider>);
navigate("/acme/issues/MUL-1");
expect(router.push).toHaveBeenCalledWith("/acme/issues/MUL-1");
});
it("ignores an event without a path", () => {
render(<WebNavigationProvider>{null}</WebNavigationProvider>);
window.dispatchEvent(new CustomEvent("multica:navigate", { detail: {} }));
expect(router.push).not.toHaveBeenCalled();
});
it("stops listening once unmounted", () => {
const { unmount } = render(
<WebNavigationProvider>{null}</WebNavigationProvider>,
);
unmount();
navigate("/acme/issues/MUL-1");
expect(router.push).not.toHaveBeenCalled();
});
});
/**
* `canGoBack` decides whether a page whose subject was just deleted steps back
* or replaces with a fallback. A wrong `true` walks the user out of Multica,
* so the adapter must expose the browser's own answer and nothing derived.
*/
describe("WebNavigationProvider canGoBack", () => {
const win = window as unknown as { navigation?: unknown };
afterEach(() => {
delete win.navigation;
});
it("passes through the Navigation API's answer", () => {
win.navigation = { canGoBack: true };
expect(renderAdapter()().canGoBack!()).toBe(true);
});
it("reads the answer live rather than freezing it at render", () => {
win.navigation = { canGoBack: true };
const adapter = renderAdapter();
win.navigation = { canGoBack: false };
expect(adapter().canGoBack!()).toBe(false);
});
// Regression (PR review, twice): a count of `push` calls stood in for real
// history here. It could not — Next drops a push to `replaceState` when the
// canonical URL is unchanged, so a push that committed no entry still made
// this claim `true`. Calling push must not move this answer at all.
it("is unmoved by a push that committed no history entry", () => {
win.navigation = { canGoBack: false };
const adapter = renderAdapter();
adapter().push("/acme/issues");
expect(router.push).toHaveBeenCalledWith("/acme/issues");
expect(adapter().canGoBack!()).toBe(false);
});
it("reports false where the browser cannot answer, so callers use the fallback", () => {
expect(renderAdapter()().canGoBack!()).toBe(false);
});
});