fix(chat): harden the ?agent= deep link against races and undetermined permissions

Review follow-up for the DM button (PR #5289):

- An explicit user action (thread select, archive, manual new chat) now
  supersedes a still-pending ?agent= intent, so an intent deferred by slow
  agent/member queries can no longer fire late and clobber that choice.
- The composingNew reset reads the live store value; under StrictMode's
  effect replay the render-captured snapshot re-closed the compose pane the
  intent had just opened (one-shot guard rightly refuses to re-fire).
- A settled miss (revoked access, archived agent, bad id) now toasts and
  strips the param instead of silently keeping a re-fireable intent; still-
  loading queries keep the intent pending. Query errors never settle.
- useAgentPermissions exposes isLoading: the detail page disables DM while
  membership resolves instead of toasting a false not_member deny at a
  legitimate member.

The chat store test mock is now reactive (useSyncExternalStore) — with a
plain mutable ref React bails out of committing post-effect re-renders and
the StrictMode regression cannot reproduce. Both new regression tests fail
against the pre-fix code.

Co-authored-by: multica-agent <github@multica.ai>
This commit is contained in:
Lambda
2026-07-12 13:45:24 +08:00
parent 6b410334f4
commit ffdea685e7
10 changed files with 250 additions and 78 deletions

View File

@@ -34,15 +34,21 @@ export function useAgentPermissions(
): {
canEdit: Decision;
canAssign: Decision;
isLoading: boolean;
} {
const { userId, role } = useCurrentMember(wsId);
const { userId, role, isLoading } = useCurrentMember(wsId);
const ctx = { userId, role };
// While the member query is in flight, `role` is null and the rules below
// would misread a legitimate member as denied (e.g. `not_member` for a
// public_to+workspace agent). Callers with always-clickable affordances
// must treat `isLoading` as "undetermined", not as a deny.
if (agent === null) {
return { canEdit: PENDING, canAssign: PENDING };
return { canEdit: PENDING, canAssign: PENDING, isLoading };
}
return {
canEdit: canEditAgent(agent, ctx),
canAssign: canAssignAgentToIssue(agent, ctx),
isLoading,
};
}

View File

@@ -29,6 +29,9 @@ vi.mock("./agent-presence-indicator", () => ({
const agentsRef = vi.hoisted(() => ({ current: [] as unknown[] }));
const membersRef = vi.hoisted(() => ({ current: [] as unknown[] }));
// When set, the member query never resolves — the "membership still loading"
// window in which the DM decision is undetermined.
const membersPendingRef = vi.hoisted(() => ({ current: false }));
const currentUserRef = vi.hoisted(() => ({
current: { id: "user-1" } as { id: string } | null,
}));
@@ -48,7 +51,10 @@ vi.mock("@multica/core/workspace/queries", () => ({
}),
memberListOptions: (wsId: string) => ({
queryKey: ["members", wsId],
queryFn: () => Promise.resolve(membersRef.current),
queryFn: () =>
membersPendingRef.current
? new Promise(() => {})
: Promise.resolve(membersRef.current),
}),
workspaceKeys: { agents: (wsId: string) => ["agents", wsId] },
}));
@@ -152,6 +158,7 @@ beforeEach(() => {
vi.clearAllMocks();
currentUserRef.current = { id: "user-1" };
membersRef.current = [{ user_id: "user-1", role: "member" }];
membersPendingRef.current = false;
agentsRef.current = [baseAgent];
});
@@ -179,6 +186,19 @@ describe("AgentDetailPage DM button", () => {
expect(push).not.toHaveBeenCalled();
});
it("disables DM while membership is resolving instead of toasting a false deny", async () => {
// Review P2: a pending member query collapses role to null, which the
// rules read as not_member — a legitimate public_to+workspace member
// would get a wrong "no access" toast. Undetermined must disable, not deny.
membersPendingRef.current = true;
const { push } = renderPage();
const dm = await screen.findByRole("button", { name: "DM" });
expect(dm).toBeDisabled();
fireEvent.click(dm);
expect(mockToastError).not.toHaveBeenCalled();
expect(push).not.toHaveBeenCalled();
});
it("hides the DM button on an archived agent", async () => {
agentsRef.current = [
{ ...baseAgent, archived_at: "2026-06-01T00:00:00Z" },

View File

@@ -107,7 +107,11 @@ export function AgentDetailPage({ agentId }: AgentDetailPageProps) {
// signature handles the not-found / loading case internally so the early
// returns below don't violate the rules of hooks. Backend gates archive
// and restore identically to edit, so a single `canEdit` covers them all.
const { canAssign, canEdit } = useAgentPermissions(agent, wsId);
const {
canAssign,
canEdit,
isLoading: permissionsLoading,
} = useAgentPermissions(agent, wsId);
const [confirmArchive, setConfirmArchive] = useState(false);
@@ -256,8 +260,11 @@ export function AgentDetailPage({ agentId }: AgentDetailPageProps) {
// Chat shares the invocation gate with assignment (MUL-3963): starting a
// chat triggers agent runs. The button stays visible either way — a denied
// click explains itself instead of the affordance silently missing.
// click explains itself instead of the affordance silently missing. While
// membership is still resolving the decision is undetermined, so the button
// is disabled rather than toasting a false "no access" at a real member.
const handleDm = () => {
if (permissionsLoading) return;
if (!canAssign.allowed) {
toast.error(t(($) => $.detail.dm_no_permission_toast));
return;
@@ -274,6 +281,7 @@ export function AgentDetailPage({ agentId }: AgentDetailPageProps) {
backHref={paths.agents()}
canAssign={canAssign.allowed}
canArchive={canEdit.allowed}
dmPending={permissionsLoading}
onDm={handleDm}
onAssign={() =>
useModalStore
@@ -380,6 +388,7 @@ function DetailHeader({
backHref,
canAssign,
canArchive,
dmPending,
onDm,
onAssign,
onArchive,
@@ -390,6 +399,7 @@ function DetailHeader({
backHref: string;
canAssign: boolean;
canArchive: boolean;
dmPending: boolean;
onDm: () => void;
onAssign: () => void;
onArchive: () => void;
@@ -457,6 +467,7 @@ function DetailHeader({
type="button"
variant="outline"
size="sm"
disabled={dmPending}
onClick={onDm}
>
<MessageSquare className="h-4 w-4" aria-hidden="true" />

View File

@@ -1,7 +1,8 @@
// @vitest-environment jsdom
import { StrictMode } from "react";
import { describe, it, expect, vi, beforeEach } from "vitest";
import { render, screen } from "@testing-library/react";
import { fireEvent, render, screen } from "@testing-library/react";
import type { Agent } from "@multica/core/types";
import { I18nProvider } from "@multica/core/i18n/react";
import enCommon from "../locales/en/common.json";
@@ -15,7 +16,9 @@ const TEST_RESOURCES = { en: { common: enCommon, chat: enChat } };
// These tests target the page-level URL wiring (`?agent=` / `?session=`), so
// the conversation internals are stubbed and the controller is replaced with
// a ref-driven fake the tests can steer.
// a ref-driven fake the tests can steer. The thread-list stub stays
// interactive: selecting a thread is the user action that must supersede a
// pending `?agent=` intent.
vi.mock("./components/chat-message-list", () => ({
ChatMessageList: () => <div>chat-message-list</div>,
ChatMessageSkeleton: () => <div>chat-message-skeleton</div>,
@@ -24,7 +27,18 @@ vi.mock("./components/chat-input", () => ({
ChatInput: () => <div>chat-input</div>,
}));
vi.mock("./components/chat-thread-list", () => ({
ChatThreadList: () => <div>chat-thread-list</div>,
ChatThreadList: ({
onSelectSession,
}: {
onSelectSession: (s: { id: string; agent_id: string }) => void;
}) => (
<button
type="button"
onClick={() => onSelectSession({ id: "session-9", agent_id: "agent-9" })}
>
select-thread
</button>
),
}));
vi.mock("./components/chat-session-header", () => ({
ChatSessionHeader: () => <div>chat-session-header</div>,
@@ -63,16 +77,34 @@ vi.mock("@multica/core/paths", () => ({
useWorkspacePaths: () => ({ chat: () => "/acme/chat" }),
}));
// The store mock is REACTIVE like real Zustand: setActiveSession replaces the
// snapshot and notifies subscribers, and the controller mock subscribes via
// useSyncExternalStore. A plain mutable ref would let React bail out of
// committing the post-effect re-render (no React state changed), leaving the
// DOM frozen on the first commit — which silently hides exactly the class of
// bug the StrictMode regression below exists to catch.
const storeRef = vi.hoisted(() => ({
current: { activeSessionId: null as string | null },
}));
const storeListeners = vi.hoisted(() => new Set<() => void>());
const availableAgentsRef = vi.hoisted(() => ({ current: [] as Agent[] }));
const agentsSettledRef = vi.hoisted(() => ({ current: true }));
const mockStartNewChat = vi.hoisted(() => vi.fn());
const mockToastError = vi.hoisted(() => vi.fn());
const mockSetActiveSession = vi.hoisted(() =>
vi.fn((id: string | null) => {
storeRef.current.activeSessionId = id;
storeRef.current = { ...storeRef.current, activeSessionId: id };
storeListeners.forEach((l) => l());
}),
);
const subscribeToStore = vi.hoisted(() => (cb: () => void) => {
storeListeners.add(cb);
return () => storeListeners.delete(cb);
});
vi.mock("sonner", () => ({
toast: { success: vi.fn(), error: mockToastError },
}));
vi.mock("@multica/core/chat", () => ({
useChatStore: Object.assign(
@@ -82,45 +114,52 @@ vi.mock("@multica/core/chat", () => ({
),
}));
vi.mock("./components/use-chat-controller", () => ({
useChatController: () => ({
wsId: "ws-1",
user: { id: "user-1" },
agents: availableAgentsRef.current,
availableAgents: availableAgentsRef.current,
sessions: [],
activeSessionId: storeRef.current.activeSessionId,
selectedAgentId: null,
currentSession: null,
isSessionArchived: false,
isAgentArchived: false,
activeAgent: availableAgentsRef.current[0] ?? null,
noAgent: false,
availability: "online",
messages: [],
pendingTask: null,
pendingTaskId: null,
showSkeleton: false,
hasMessages: false,
firstItemIndex: 0,
hasOlderMessages: false,
isFetchingOlderMessages: false,
fetchOlderMessages: vi.fn(),
restoreDraftRequest: null,
handleRestoreDraftConsumed: vi.fn(),
focusInputRequest: 0,
handleSend: vi.fn(),
handleStop: vi.fn(),
handleUploadFile: vi.fn(),
handleNewChat: vi.fn(),
handleStartNewChat: mockStartNewChat,
handleSelectSession: vi.fn(),
advanceSelectionAfterArchive: vi.fn(),
archiveSession: vi.fn(),
setActiveSession: mockSetActiveSession,
setSelectedAgentId: vi.fn(),
}),
}));
vi.mock("./components/use-chat-controller", async () => {
const { useSyncExternalStore } = await import("react");
return {
useChatController: () => ({
wsId: "ws-1",
user: { id: "user-1" },
agents: availableAgentsRef.current,
availableAgents: availableAgentsRef.current,
agentsSettled: agentsSettledRef.current,
sessions: [],
activeSessionId: useSyncExternalStore(
subscribeToStore,
() => storeRef.current,
).activeSessionId,
selectedAgentId: null,
currentSession: null,
isSessionArchived: false,
isAgentArchived: false,
activeAgent: availableAgentsRef.current[0] ?? null,
noAgent: false,
availability: "online",
messages: [],
pendingTask: null,
pendingTaskId: null,
showSkeleton: false,
hasMessages: false,
firstItemIndex: 0,
hasOlderMessages: false,
isFetchingOlderMessages: false,
fetchOlderMessages: vi.fn(),
restoreDraftRequest: null,
handleRestoreDraftConsumed: vi.fn(),
focusInputRequest: 0,
handleSend: vi.fn(),
handleStop: vi.fn(),
handleUploadFile: vi.fn(),
handleNewChat: vi.fn(),
handleStartNewChat: mockStartNewChat,
handleSelectSession: vi.fn(),
advanceSelectionAfterArchive: vi.fn(),
archiveSession: vi.fn(),
setActiveSession: mockSetActiveSession,
setSelectedAgentId: vi.fn(),
}),
};
});
import { ChatPage } from "./chat-page";
@@ -149,7 +188,9 @@ const agent: Agent = {
archived_by: null,
};
function renderPage(search: string) {
const NO_ACCESS_MSG = "You don't have access to chat with this agent.";
function renderPage(search: string, { strict = false } = {}) {
const replace = vi.fn();
const navigation: NavigationAdapter = {
push: vi.fn(),
@@ -161,13 +202,16 @@ function renderPage(search: string) {
};
// A fresh element per render — reusing one element object lets React bail
// out of re-rendering, which would make the rerender-based tests vacuous.
const makeUi = () => (
<I18nProvider locale="en" resources={TEST_RESOURCES}>
<NavigationProvider value={navigation}>
<ChatPage />
</NavigationProvider>
</I18nProvider>
);
const makeUi = () => {
const page = (
<I18nProvider locale="en" resources={TEST_RESOURCES}>
<NavigationProvider value={navigation}>
<ChatPage />
</NavigationProvider>
</I18nProvider>
);
return strict ? <StrictMode>{page}</StrictMode> : page;
};
const view = render(makeUi());
return { replace, rerender: () => view.rerender(makeUi()) };
}
@@ -175,7 +219,9 @@ function renderPage(search: string) {
beforeEach(() => {
vi.clearAllMocks();
storeRef.current = { activeSessionId: null };
storeListeners.clear();
availableAgentsRef.current = [agent];
agentsSettledRef.current = true;
});
describe("ChatPage ?agent= deep link", () => {
@@ -199,22 +245,65 @@ describe("ChatPage ?agent= deep link", () => {
expect(mockStartNewChat).toHaveBeenCalledTimes(1);
});
it("stays pending without a toast while the agent/member queries load", () => {
availableAgentsRef.current = [];
agentsSettledRef.current = false;
const { replace } = renderPage("agent=agent-1");
expect(mockStartNewChat).not.toHaveBeenCalled();
expect(mockToastError).not.toHaveBeenCalled();
expect(replace).not.toHaveBeenCalled();
});
it("waits for the agent list to resolve before consuming the intent", () => {
availableAgentsRef.current = [];
agentsSettledRef.current = false;
const { replace, rerender } = renderPage("agent=agent-1");
expect(mockStartNewChat).not.toHaveBeenCalled();
expect(replace).not.toHaveBeenCalled();
availableAgentsRef.current = [agent];
agentsSettledRef.current = true;
rerender();
expect(mockStartNewChat).toHaveBeenCalledWith(agent);
expect(replace).toHaveBeenCalledWith("/acme/chat");
});
it("ignores an agent id that is not chat-able and keeps the default view", () => {
availableAgentsRef.current = [agent];
const { replace } = renderPage("agent=other-agent");
it("toasts and strips the param once the list settles without the agent", () => {
// Revoked access, archived agent, or a bad id: a settled miss must
// explain itself and consume the intent so a later refetch that surfaces
// the agent cannot auto-start a chat without a fresh click.
const { replace, rerender } = renderPage("agent=other-agent");
expect(mockStartNewChat).not.toHaveBeenCalled();
expect(replace).not.toHaveBeenCalled();
expect(mockToastError).toHaveBeenCalledWith(NO_ACCESS_MSG);
expect(replace).toHaveBeenCalledWith("/acme/chat");
expect(screen.queryByText("chat-input")).not.toBeInTheDocument();
rerender();
expect(mockToastError).toHaveBeenCalledTimes(1);
});
it("lets an explicit thread selection supersede a still-pending intent", () => {
// Deferred-intent race (review P1): the user picks a thread while the
// agent/member queries are still loading; when they settle, the stale
// deep link must NOT fire and clobber that selection.
availableAgentsRef.current = [];
agentsSettledRef.current = false;
const { replace, rerender } = renderPage("agent=agent-1");
fireEvent.click(screen.getByRole("button", { name: "select-thread" }));
availableAgentsRef.current = [agent];
agentsSettledRef.current = true;
rerender();
expect(mockStartNewChat).not.toHaveBeenCalled();
expect(mockToastError).not.toHaveBeenCalled();
expect(replace).not.toHaveBeenCalled();
});
it("opens the compose pane under StrictMode with a persisted previous session", () => {
// StrictMode replays mount effects with render-captured values (review
// P2): the composingNew reset must read the LIVE store value, or the
// stale persisted session re-closes the pane the intent just opened —
// while the consumed-intent guard rightly refuses to fire again.
storeRef.current = { activeSessionId: "old-session" };
const { replace } = renderPage("agent=agent-1", { strict: true });
expect(mockStartNewChat).toHaveBeenCalledTimes(1);
expect(replace).toHaveBeenCalledWith("/acme/chat");
expect(screen.getByText("chat-input")).toBeInTheDocument();
});
});

View File

@@ -3,6 +3,7 @@
import { useEffect, useRef, useState } from "react";
import { useDefaultLayout } from "react-resizable-panels";
import { ArrowLeft, MessageSquare } from "lucide-react";
import { toast } from "sonner";
import { Button } from "@multica/ui/components/ui/button";
import {
ResizablePanelGroup,
@@ -62,7 +63,12 @@ export function ChatPage() {
// real session takes over.
const [composingNew, setComposingNew] = useState(false);
useEffect(() => {
if (c.activeSessionId) setComposingNew(false);
// Read the LIVE store value for the same reason as the session sync
// effects below: under StrictMode's double-invoke this effect replays
// with the render-captured snapshot, and a stale non-null session (a
// persisted chat the URL→store effect already cleared) would revert the
// composingNew=true that the later `?agent=` intent effect just set.
if (useChatStore.getState().activeSessionId) setComposingNew(false);
}, [c.activeSessionId]);
// Two-way sync between the URL (`?session=`) and the chat store's
@@ -98,7 +104,20 @@ export function ChatPage() {
id: "multica_chat_layout",
});
// `?agent=` intent bookkeeping. The ref holds the param value already
// consumed (or superseded) so the effect below fires at most once per deep
// link — it also bridges the async window between replace() and the
// searchParams actually dropping the param. Any explicit user action must
// supersede a still-pending intent: agent/member queries can resolve late,
// and a deferred intent firing after the user picked a thread (or started
// another chat) would clobber that choice.
const consumedAgentIntent = useRef<string | null>(null);
const supersedeAgentIntent = () => {
if (urlAgent) consumedAgentIntent.current = urlAgent;
};
const handleSelect = (session: ChatSession) => {
supersedeAgentIntent();
c.handleSelectSession(session);
setComposingNew(false);
};
@@ -109,6 +128,7 @@ export function ChatPage() {
// the list, which reads more naturally than being thrown into an unrelated
// conversation full-screen. Archiving any other chat leaves the view put.
const handleArchive = (session: ChatSession) => {
supersedeAgentIntent();
if (session.id === c.activeSessionId) {
if (isMobile) {
c.setActiveSession(null);
@@ -121,6 +141,9 @@ export function ChatPage() {
};
const startNewChat = (agent: Agent | null) => {
// A manual ⊕ pick outranks a pending deep link; when called FROM the
// intent effect the ref is already set to this param, so this is a no-op.
supersedeAgentIntent();
if (agent) c.handleStartNewChat(agent);
else c.handleNewChat();
setComposingNew(true);
@@ -131,11 +154,11 @@ export function ChatPage() {
// agent. The permission-filtered agent list loads async, so the intent is
// consumed on the render where the agent resolves, then the param is
// stripped so refresh / the session sync above don't replay it. The ref
// guards the window between replace() and the searchParams actually
// updating; it resets once the param is gone so a later identical deep link
// fires again. An id that never resolves (access revoked or agent archived
// in the meantime) is ignored and the page stays on its default view.
const consumedAgentIntent = useRef<string | null>(null);
// resets once the param is gone so a later identical deep link fires again.
// A settled miss (access revoked, agent archived, bad id) is a denial: it
// explains itself with a toast and consumes the intent so a later refetch
// that surfaces the agent cannot start a chat without a fresh click. While
// the queries are still loading the intent simply stays pending.
useEffect(() => {
if (!urlAgent) {
consumedAgentIntent.current = null;
@@ -143,12 +166,19 @@ export function ChatPage() {
}
if (consumedAgentIntent.current === urlAgent) return;
const agent = c.availableAgents.find((a) => a.id === urlAgent);
if (!agent) return;
consumedAgentIntent.current = urlAgent;
startNewChat(agent);
replace(wsPaths.chat());
if (agent) {
consumedAgentIntent.current = urlAgent;
startNewChat(agent);
replace(wsPaths.chat());
return;
}
if (c.agentsSettled) {
consumedAgentIntent.current = urlAgent;
toast.error(t(($) => $.page.agent_link_no_access));
replace(wsPaths.chat());
}
// eslint-disable-next-line react-hooks/exhaustive-deps -- consume when the URL param or the resolving agent list changes
}, [urlAgent, c.availableAgents]);
}, [urlAgent, c.availableAgents, c.agentsSettled]);
const newChatButton = (
<NewChatButton

View File

@@ -203,8 +203,12 @@ export function useChatController(opts?: { isActive?: boolean }) {
const setActiveSession = useChatStore((s) => s.setActiveSession);
const setSelectedAgentId = useChatStore((s) => s.setSelectedAgentId);
const user = useAuthStore((s) => s.user);
const { data: agents = [] } = useQuery(agentListOptions(wsId));
const { data: members = [] } = useQuery(memberListOptions(wsId));
const { data: agents = [], isSuccess: agentsLoaded } = useQuery(
agentListOptions(wsId),
);
const { data: members = [], isSuccess: membersLoaded } = useQuery(
memberListOptions(wsId),
);
const { data: sessions = [], isSuccess: sessionsLoaded } = useQuery(
chatSessionsOptions(wsId),
);
@@ -265,6 +269,13 @@ export function useChatController(opts?: { isActive?: boolean }) {
const availableAgents = agents.filter(
(a) => !a.archived_at && canAssignAgent(a, user?.id, memberRole),
);
// `availableAgents` is only trustworthy once BOTH queries above succeeded:
// the permission filter reads the member role, so agents-without-members
// misreports a public_to agent as unavailable. Consumers that must tell
// "still loading" apart from "settled and not available" (the `?agent=`
// deep link) gate on this instead of sniffing list emptiness. Query errors
// deliberately keep this false — a failed fetch is not a permission verdict.
const agentsSettled = agentsLoaded && membersLoaded;
// The agent bound to the OPEN session, resolved from the full agent list
// (archived included, since agentListOptions passes include_archived). An
@@ -655,6 +666,7 @@ export function useChatController(opts?: { isActive?: boolean }) {
user,
agents,
availableAgents,
agentsSettled,
sessions,
activeSessionId,
selectedAgentId,

View File

@@ -1,7 +1,8 @@
{
"page": {
"title": "Chat",
"select_prompt": "Pick a conversation, or start a new one with +"
"select_prompt": "Pick a conversation, or start a new one with +",
"agent_link_no_access": "You don't have access to chat with this agent."
},
"fab": {
"running": "Multica is working...",

View File

@@ -1,7 +1,8 @@
{
"page": {
"title": "チャット",
"select_prompt": "会話を選ぶか、+ で新規作成してください"
"select_prompt": "会話を選ぶか、+ で新規作成してください",
"agent_link_no_access": "このエージェントとチャットする権限がありません。"
},
"fab": {
"running": "Multica が作業中です...",

View File

@@ -1,7 +1,8 @@
{
"page": {
"title": "채팅",
"select_prompt": "대화를 선택하거나 +로 새로 시작하세요"
"select_prompt": "대화를 선택하거나 +로 새로 시작하세요",
"agent_link_no_access": "이 에이전트와 채팅할 권한이 없습니다."
},
"fab": {
"running": "Multica가 작업 중입니다...",

View File

@@ -1,7 +1,8 @@
{
"page": {
"title": "聊天",
"select_prompt": "选择一个对话,或点 + 新建"
"select_prompt": "选择一个对话,或点 + 新建",
"agent_link_no_access": "你没有权限与该智能体聊天。"
},
"fab": {
"running": "Multica 正在工作...",