diff --git a/AGENTS.md b/AGENTS.md index 1b4e1726f..73bf857b1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -46,6 +46,8 @@ this list is only a reminder. ([architecture §3](./docs/FRONTEND_ARCHITECTURE.md#3-source-layout-src)). - A new protected route needs an `AppRoute` entry and a `routePermissions` entry ([architecture §4](./docs/FRONTEND_ARCHITECTURE.md#4-routing--access-control)). +- Added, renamed or removed a page, tab or quoted label? Update the buddy's app guide + in the backend too ([architecture §4.3](./docs/FRONTEND_ARCHITECTURE.md#43-access-policy-srcauthaccesspolicyts)). - Use the `ui/` primitives and the palette tokens ([coding standards §4](./docs/FRONTEND_CODING_STANDARDS.md#4-styling-tailwind-css-v4)). - Touching a shared primitive or token? Check the Keycloak login theme's copy diff --git a/docs/FRONTEND_ARCHITECTURE.md b/docs/FRONTEND_ARCHITECTURE.md index 0ac61a5bb..cccc1081c 100644 --- a/docs/FRONTEND_ARCHITECTURE.md +++ b/docs/FRONTEND_ARCHITECTURE.md @@ -207,6 +207,13 @@ Route-level authorization is centralized in `src/auth/accessPolicy.ts`: > **New protected routes must be added to `AppRoute` + `routePermissions`**, or they > won't type-check and won't be access-controlled. +> **The buddy has a hand-written map of these pages** (`AppGuide.kt` in the backend, +> served by the `get_app_guide` tool). A new, renamed or removed page, tab or tab URL +> parameter, or a changed sidebar/tab/button label it quotes, means updating that file +> in the same change. `appGuideRoutes.test.ts` fails when `accessPolicy.ts` gains a +> route the guide neither describes nor excludes on purpose +> (`src/features/buddy/appGuideRoutes.ts`). + ### 4.4 Actual route list Declared in `AppRouter.tsx`: diff --git a/src/auth/accessPolicy.ts b/src/auth/accessPolicy.ts index 55ab273c4..ff0697cca 100644 --- a/src/auth/accessPolicy.ts +++ b/src/auth/accessPolicy.ts @@ -166,6 +166,12 @@ export function getDefaultRoute(profile: UserProfile | null): AppRoute { return "/"; } +/** + * Every route the policy knows, at runtime. `AppRoute` is a type and vanishes on build, so anything + * that has to enumerate the routes — the buddy's app-guide coverage test, say — reads this. + */ +export const APP_ROUTES = Object.keys(routePermissions) as AppRoute[]; + /** * Maps a real pathname to the `AppRoute` whose permissions apply to it. * @@ -175,7 +181,7 @@ export function getDefaultRoute(profile: UserProfile | null): AppRoute { * e.g. an unknown URL. */ export function getMatchingProtectedRoute(pathname: string): AppRoute | null { - const routes = Object.keys(routePermissions) as AppRoute[]; + const routes = APP_ROUTES; const exactMatch = routes.find((route) => route === pathname); diff --git a/src/features/buddy/appGuideRoutes.ts b/src/features/buddy/appGuideRoutes.ts new file mode 100644 index 000000000..a9c42241a --- /dev/null +++ b/src/features/buddy/appGuideRoutes.ts @@ -0,0 +1,42 @@ +import type { AppRoute } from "../../auth/accessPolicy"; + +/** + * The routes the buddy's app guide describes — a mirror of the page list in the backend's + * `onboarding/service/AppGuide.kt`, which is what the `get_app_guide` tool reads from. + * + * The guide lives in the backend because it is filtered by the caller's role there, but it is a + * description of *this* app, and it goes stale the moment a page changes here. This list is how a + * change here notices: `appGuideRoutes.test.ts` fails when `accessPolicy.ts` has a route that is + * neither in this list nor in {@link APP_GUIDE_EXCLUDED_ROUTES}. Making it pass means deciding — + * describe the page in `AppGuide.kt` (and list it here), or exclude it here with a reason. + * + * Not checked automatically across the repos: adding a route here does not prove the backend + * describes it. The two lists are kept in the same order as `AppGuide.kt` to make that review a + * glance. Member profiles (`/team/:userId`) are described there too, under Team. + */ +export const APP_GUIDE_ROUTES: readonly AppRoute[] = [ + "/", + "/board", + "/chat", + "/buddy", + "/knowledge-base", + "/onboarding", + "/settings", + "/pm-dashboard", + "/team-management", + "/insights/onboarding", + "/insights/faq", + "/insights/knowledge-gaps", + "/insights/knowledge-requests", + "/data-ingestion", + "/blueprints", + "/hire-setup", + "/admin", +]; + +/** Routes the guide leaves out on purpose, and why — each one a decision, not an oversight. */ +export const APP_GUIDE_EXCLUDED_ROUTES: Partial> = { + "/arrival-steps": "Redirects to /hire-setup?tab=arrival.", + "/starter-work": "Redirects to /hire-setup?tab=starter.", + "/profile": "Redirects to /settings.", +}; diff --git a/src/features/buddy/hooks/useBuddyConversation.ts b/src/features/buddy/hooks/useBuddyConversation.ts index 5d64fa730..6993e93ce 100644 --- a/src/features/buddy/hooks/useBuddyConversation.ts +++ b/src/features/buddy/hooks/useBuddyConversation.ts @@ -1112,6 +1112,11 @@ export function useBuddyConversation( // Read at call time: a turn speaks to whichever conversation is current when it starts. sessionId, teamProjectIdRef.current ?? undefined, + // The page the dock is floating over, for the buddy's app guide. Read from the window at + // send time rather than through `useLocation`, which would re-render the one app-wide + // conversation on every navigation to learn something only a send needs. The pathname + // is all the guide matches on, so the query string stays on this side. + window.location.pathname, ); // The turn is over — the first moment a card placed mid-answer is certainly on the board. diff --git a/src/features/buddy/toolLabel.ts b/src/features/buddy/toolLabel.ts index 37c0fdb14..e3bded4c7 100644 --- a/src/features/buddy/toolLabel.ts +++ b/src/features/buddy/toolLabel.ts @@ -7,6 +7,8 @@ const TOOL_LABELS: Record = { get_my_metrics: "Checking your progress…", get_my_competencies: "Looking at where you stand…", get_suggested_tasks: "Finding good tasks for you…", + // Mounted in both modes: everybody asks where things are in the app. + get_app_guide: "Checking where that is in the app…", // Team-mode reads: the manager is asking about their *people*, and the labels say so. get_team_attention: "Seeing who needs attention…", diff --git a/src/pages/AdminPage.tsx b/src/pages/AdminPage.tsx index f28736fd4..0f367aebd 100644 --- a/src/pages/AdminPage.tsx +++ b/src/pages/AdminPage.tsx @@ -87,13 +87,17 @@ export function AdminPage() { A hand-off, not a permanent part of the URL: it seeds the tab once and is then stripped, so the tab bar keeps behaving exactly as before and the back button does not turn into a step-through of sections. + + Consumed whenever it arrives, not only on mount: the buddy links to `/admin?tab=projects` + in place, and somebody already on this page would otherwise keep their tab while the + parameter sat in the URL. */ const [searchParams, setSearchParams] = useSearchParams(); /* `?projectId=` and `?userId=` open that project's or user's drawer, so a link from elsewhere (a dashboard card, a message) can land on the thing it talks about. They are handed off - exactly like `tab`: read once, then stripped. The drawer itself opens once the lists have - loaded, since it needs the record to show. + like `tab`, but only on mount: the drawer logic below assumes no drawer is open yet. The + drawer itself opens once the lists have loaded, since it needs the record to show. */ const [pendingDeepLink, setPendingDeepLink] = useState<{ projectId: string | null; @@ -104,22 +108,25 @@ export function AdminPage() { return projectId || userId ? { projectId, userId } : null; }); - const [activeTab, setActiveTab] = useState(() => { - const requested = searchParams.get("tab"); - const requestedTab = visibleAdminTabs.find((tab) => tab === requested); - - if (requestedTab) return requestedTab; - - return searchParams.get("projectId") ? "projects" : "users"; - }); + const requestedTab = visibleAdminTabs.find((tab) => tab === searchParams.get("tab")); + const [activeTab, setActiveTab] = useState( + () => requestedTab ?? (searchParams.get("projectId") ? "projects" : "users"), + ); + // Adjusted during render rather than in the effect below: the tab follows a new request in + // the same paint, and forgetting the request once stripped lets the same link work twice. + const [handledTab, setHandledTab] = useState(requestedTab); + if (requestedTab !== handledTab) { + setHandledTab(requestedTab); + if (requestedTab) setActiveTab(requestedTab); + } - const tabParamConsumed = useRef(false); + const drawerParamsConsumed = useRef(false); useEffect(() => { - if (tabParamConsumed.current) return; - tabParamConsumed.current = true; + const keys = drawerParamsConsumed.current ? ["tab"] : ["tab", "projectId", "userId"]; + drawerParamsConsumed.current = true; - const handedOff = ["tab", "projectId", "userId"].filter((key) => searchParams.has(key)); + const handedOff = keys.filter((key) => searchParams.has(key)); if (handedOff.length === 0) return; const next = new URLSearchParams(searchParams); diff --git a/src/services/buddyService.ts b/src/services/buddyService.ts index 6c221af52..316915561 100644 --- a/src/services/buddyService.ts +++ b/src/services/buddyService.ts @@ -493,18 +493,25 @@ async function readBuddyStream( * @param teamProjectId - Pass to speak in *team mode* about a managed project instead of the hire's * own conversation. Sent in the body, not the query string — the backend's contract puts the * team target on the POST body and leaves the hire's own conversation to `sessionId`. + * @param currentPage The app path the sender is on (`/team-management`), so the buddy's app + * guide can answer "where is this here?". Omitted when unknown, like `teamProjectId`. */ export async function streamMessage( content: string, handlers: BuddyStreamHandlers, sessionId: string | undefined, teamProjectId?: string, + currentPage?: string, ): Promise { const outcome = await readBuddyStream( `/api/v1/onboarding/me/buddy/messages`, // Omitted, never null: a team target replaces the conversation, and carrying // `teamProjectId: null` would send a field the contract does not have. - teamProjectId ? { content, teamProjectId } : { content, sessionId }, + { + content, + ...(teamProjectId ? { teamProjectId } : { sessionId }), + ...(currentPage ? { currentPage } : {}), + }, (event) => { switch (event.type) { case "tool_use": diff --git a/tests/unit/features/buddy/appGuideRoutes.test.ts b/tests/unit/features/buddy/appGuideRoutes.test.ts new file mode 100644 index 000000000..3399482d1 --- /dev/null +++ b/tests/unit/features/buddy/appGuideRoutes.test.ts @@ -0,0 +1,34 @@ +import { describe, expect, it } from "vitest"; +import { APP_ROUTES } from "../../../../src/auth/accessPolicy"; +import { + APP_GUIDE_EXCLUDED_ROUTES, + APP_GUIDE_ROUTES, +} from "../../../../src/features/buddy/appGuideRoutes"; + +/** + * The buddy's map of the app lives in the backend (`AppGuide.kt`). This is the tripwire on this + * side: a page added to the access policy without a decision about the guide fails here, in the + * repo the page was added in. + */ +describe("the buddy's app guide coverage", () => { + it("covers or deliberately excludes every route in the access policy", () => { + const decided = new Set([ + ...APP_GUIDE_ROUTES, + ...Object.keys(APP_GUIDE_EXCLUDED_ROUTES), + ]); + + // When this fails: describe the new page in the backend's onboarding/service/AppGuide.kt and + // add it to APP_GUIDE_ROUTES, or add it to APP_GUIDE_EXCLUDED_ROUTES with the reason. + expect(APP_ROUTES.filter((route) => !decided.has(route))).toEqual([]); + }); + + it("never lists a route both as described and as excluded", () => { + const excluded = Object.keys(APP_GUIDE_EXCLUDED_ROUTES); + + expect(APP_GUIDE_ROUTES.filter((route) => excluded.includes(route))).toEqual([]); + }); + + it("names only routes the access policy knows", () => { + expect(APP_GUIDE_ROUTES.filter((route) => !APP_ROUTES.includes(route))).toEqual([]); + }); +}); diff --git a/tests/unit/features/buddy/toolLabel.test.ts b/tests/unit/features/buddy/toolLabel.test.ts index bc1a4cadf..41f545b6c 100644 --- a/tests/unit/features/buddy/toolLabel.test.ts +++ b/tests/unit/features/buddy/toolLabel.test.ts @@ -6,6 +6,7 @@ describe("toolLabel", () => { expect(toolLabel("get_my_metrics")).toBe("Checking your progress…"); expect(toolLabel("get_my_competencies")).toBe("Looking at where you stand…"); expect(toolLabel("get_suggested_tasks")).toBe("Finding good tasks for you…"); + expect(toolLabel("get_app_guide")).toBe("Checking where that is in the app…"); }); it("falls back to a generic label for an unknown tool", () => { diff --git a/tests/unit/pages/AdminPage.test.tsx b/tests/unit/pages/AdminPage.test.tsx index ffa9ebcf4..d6d104835 100644 --- a/tests/unit/pages/AdminPage.test.tsx +++ b/tests/unit/pages/AdminPage.test.tsx @@ -1,7 +1,7 @@ import { render, screen, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { describe, it, expect, vi, beforeEach } from "vitest"; -import { MemoryRouter, useLocation } from "react-router-dom"; +import { MemoryRouter, useLocation, useNavigate } from "react-router-dom"; import { AdminPage } from "../../../src/pages/AdminPage"; import { DRAWER_CLOSE_DELAY_MS } from "../../../src/features/admin/data"; import type { AdminUser, ProjectSummary } from "../../../src/services/adminUserService"; @@ -244,6 +244,30 @@ describe("AdminPage", () => { expect(screen.queryByTestId("users-tab")).not.toBeInTheDocument(); }); + // The buddy links to `/admin?tab=projects` in place, so the reader may already be here. + it("follows a tab named in the URL while the page is already open", async () => { + function BuddyLink() { + const navigate = useNavigate(); + return ; + } + const user = userEvent.setup(); + render( + + + + , + ); + await waitFor(() => { + expect(screen.getByTestId("users-tab")).toBeInTheDocument(); + }); + + await user.click(screen.getByRole("button", { name: "buddy link" })); + + await waitFor(() => { + expect(screen.getByTestId("projects-tab")).toBeInTheDocument(); + }); + }); + it("ignores a tab in the URL that does not exist", async () => { render( diff --git a/tests/unit/services/buddyService.test.ts b/tests/unit/services/buddyService.test.ts index c1ade890b..1b5f989d3 100644 --- a/tests/unit/services/buddyService.test.ts +++ b/tests/unit/services/buddyService.test.ts @@ -732,6 +732,32 @@ describe("buddyService", () => { expect(capturedBody).toEqual({ content: "who is behind?", teamProjectId: "p-123" }); }); + it("sends the page the hire is on, and omits it when unknown", async () => { + const bodies: Record[] = []; + const encoder = new TextEncoder(); + server.use( + http.post("/api/v1/onboarding/me/buddy/messages", async ({ request }) => { + bodies.push((await request.json()) as Record); + const stream = new ReadableStream({ + start(controller) { + controller.enqueue(encoder.encode('data: {"type":"done"}\n\n')); + controller.close(); + }, + }); + return new HttpResponse(stream, { headers: { "Content-Type": "text/event-stream" } }); + }), + ); + const handlers = { onToken: vi.fn(), onCitation: vi.fn(), onDone: vi.fn() }; + + await streamMessage("where are roles?", handlers, "session-1", undefined, "/team-management"); + await streamMessage("hi", handlers, "session-1"); + + expect(bodies).toEqual([ + { content: "where are roles?", sessionId: "session-1", currentPage: "/team-management" }, + { content: "hi", sessionId: "session-1" }, + ]); + }); + it("opens the team greeting under the teamProjectId query param", async () => { let capturedUrl = ""; const encoder = new TextEncoder();