Repository navigation
fix(desktop): stop the page watcher menu claiming nothing watches a page #7724
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.
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
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
1b5bf87
fix(desktop): stop the page watcher menu claiming nothing watches a page
harshithmullapudi 6042209
fix(i18n): match the page watcher strings to each catalog's own terms
harshithmullapudi 0fc74e3
feat(desktop): show the agents watching a page, not a sentence about …
harshithmullapudi 05548d2
fix(desktop): give the watcher row room for the session name
harshithmullapudi 68e0157
fix(desktop): stop a finished watch lingering as "on a host you can't…
saddlepaddle 8cf8d67
Merge main into super-2406 (resolve page-watch-manager against #7743)
saddlepaddle File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
5 changes: 5 additions & 0 deletions
5
apps/desktop/src/renderer/hooks/host-service/usePageWatchersForPage/index.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| export { | ||
| getHostPageWatchersQueryKey, | ||
| type PageWatcherRow, | ||
| usePageWatchersForPage, | ||
| } from "./usePageWatchersForPage"; |
119 changes: 119 additions & 0 deletions
119
.../desktop/src/renderer/hooks/host-service/usePageWatchersForPage/usePageWatchersForPage.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| import { useQueries, useQueryClient } from "@tanstack/react-query"; | ||
| import { useCallback, useMemo } from "react"; | ||
| import { cloudTrpc } from "renderer/lib/cloud-trpc"; | ||
| import { getHostServiceClientByUrl } from "renderer/lib/host-service-client"; | ||
| import { useHostWorkspaces } from "renderer/routes/_authenticated/providers/HostWorkspacesProvider"; | ||
| import { useWorkspaceEvent } from "../useWorkspaceEvent"; | ||
|
|
||
| const REFRESH_MS = 30_000; | ||
|
|
||
| export interface PageWatcherRow { | ||
| workspaceId: string; | ||
| workspaceName: string | null; | ||
| terminalId: string; | ||
| agentId: string | null; | ||
| sessionTitle: string | null; | ||
| hostId: string; | ||
| } | ||
|
|
||
| export function getHostPageWatchersQueryKey(hostUrl: string) { | ||
| return ["page-watchers-by-host", hostUrl] as const; | ||
| } | ||
|
|
||
| interface HostTarget { | ||
| hostId: string; | ||
| hostUrl: string; | ||
| } | ||
|
|
||
| /** | ||
| * Every agent watching `pageId`, across the hosts this machine can already | ||
| * reach, with the workspace each one sits in. | ||
| * | ||
| * A host's in-memory list is the watch truth, so a host that answers is | ||
| * reporting live state — there is nothing to age out and no heartbeat to | ||
| * read. The cost is that a watcher on a host we cannot reach is not in here; | ||
| * the page's org-wide watch flag still says one exists, and the menu says so. | ||
| */ | ||
| export function usePageWatchersForPage({ | ||
| pageId, | ||
| workspaceId, | ||
| }: { | ||
| pageId: string | undefined; | ||
| workspaceId: string; | ||
| }): PageWatcherRow[] { | ||
| const { workspaces, cache } = useHostWorkspaces(); | ||
| const queryClient = useQueryClient(); | ||
| const cloudUtils = cloudTrpc.useUtils(); | ||
|
|
||
| const currentHostId = useMemo( | ||
| () => workspaces.find((workspace) => workspace.id === workspaceId)?.hostId, | ||
| [workspaces, workspaceId], | ||
| ); | ||
|
|
||
| // A sandbox counts every request as activity, so polling one in the | ||
| // background would hold a cloud workspace's VM awake for as long as the app | ||
| // is open. Only the sandbox already on screen is asked. | ||
| const targets = useMemo<HostTarget[]>(() => { | ||
| const seen = new Map<string, HostTarget>(); | ||
| for (const workspace of workspaces) { | ||
| const hostId = workspace.hostId; | ||
| if (seen.has(hostId)) continue; | ||
| if (cache.isSandboxHost(hostId) && hostId !== currentHostId) continue; | ||
| const hostUrl = cache.resolveHostUrl(hostId); | ||
| if (!hostUrl) continue; | ||
| seen.set(hostId, { hostId, hostUrl }); | ||
| } | ||
| return [...seen.values()]; | ||
| }, [workspaces, cache, currentHostId]); | ||
|
|
||
| const results = useQueries({ | ||
| queries: targets.map((target) => ({ | ||
| queryKey: getHostPageWatchersQueryKey(target.hostUrl), | ||
| enabled: Boolean(pageId), | ||
| staleTime: REFRESH_MS, | ||
| refetchInterval: REFRESH_MS, | ||
| queryFn: async () => | ||
| await getHostServiceClientByUrl(target.hostUrl).pageWatch.getAll.query( | ||
| {}, | ||
| ), | ||
| })), | ||
| }); | ||
|
|
||
| useWorkspaceEvent( | ||
| "page-watch:changed", | ||
| workspaceId, | ||
| useCallback(() => { | ||
| void queryClient.invalidateQueries({ | ||
| queryKey: ["page-watchers-by-host"], | ||
| }); | ||
| if (pageId) void cloudUtils.page.get.invalidate({ id: pageId }); | ||
| }, [queryClient, cloudUtils, pageId]), | ||
| ); | ||
|
|
||
| const names = useMemo( | ||
| () => | ||
| new Map(workspaces.map((workspace) => [workspace.id, workspace.name])), | ||
| [workspaces], | ||
| ); | ||
|
|
||
| return useMemo(() => { | ||
| if (!pageId) return []; | ||
| const rows: PageWatcherRow[] = []; | ||
| for (const [index, result] of results.entries()) { | ||
| const target = targets[index]; | ||
| if (!target) continue; | ||
| for (const watcher of result.data ?? []) { | ||
| if (watcher.pageId !== pageId) continue; | ||
| rows.push({ | ||
| workspaceId: watcher.workspaceId, | ||
| workspaceName: names.get(watcher.workspaceId) ?? null, | ||
| terminalId: watcher.terminalId, | ||
| agentId: watcher.agentId, | ||
| sessionTitle: watcher.sessionTitle, | ||
| hostId: target.hostId, | ||
| }); | ||
| } | ||
| } | ||
| return rows; | ||
| }, [results, targets, names, pageId]); | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
205 changes: 205 additions & 0 deletions
205
...istry/components/PagePaneHeaderExtras/components/PageWatcherMenu/PageWatcherMenu.test.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,205 @@ | ||
| import { | ||
| afterAll, | ||
| afterEach, | ||
| beforeEach, | ||
| describe, | ||
| expect, | ||
| mock, | ||
| test, | ||
| } from "bun:test"; | ||
| import { GlobalRegistrator } from "@happy-dom/global-registrator"; | ||
|
|
||
| const alreadyRegistered = GlobalRegistrator.isRegistered; | ||
| if (!alreadyRegistered) GlobalRegistrator.register(); | ||
| ( | ||
| globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean } | ||
| ).IS_REACT_ACT_ENVIRONMENT = true; | ||
|
|
||
| const PAGE_ID = "5d3f2a1e-0c7b-4a2d-9f11-6b8c0d4e7a52"; | ||
| const WORKSPACE_ID = "ws-local"; | ||
|
|
||
| interface Row { | ||
| workspaceId: string; | ||
| workspaceName: string | null; | ||
| terminalId: string; | ||
| agentId: string | null; | ||
| sessionTitle: string | null; | ||
| hostId: string; | ||
| } | ||
| type CloudWatch = { watching: boolean; agentId: string | null }; | ||
|
|
||
| let rows: Row[] = []; | ||
| let cloudWatch: CloudWatch = { watching: false, agentId: null }; | ||
| let navigated: Array<{ workspaceId: string; terminalId: string | undefined }> = | ||
| []; | ||
|
|
||
| mock.module("renderer/hooks/host-service/usePageWatchersForPage", () => ({ | ||
| usePageWatchersForPage: () => rows, | ||
| })); | ||
| mock.module("renderer/lib/cloud-trpc", () => ({ | ||
| cloudTrpc: { | ||
| page: { get: { useQuery: () => ({ data: { watch: cloudWatch } }) } }, | ||
| }, | ||
| })); | ||
| mock.module("@tanstack/react-router", () => ({ | ||
| useNavigate: () => () => Promise.resolve(), | ||
| })); | ||
| mock.module( | ||
| "renderer/routes/_authenticated/_dashboard/utils/workspace-navigation", | ||
| () => ({ | ||
| navigateToV2Workspace: ( | ||
| workspaceId: string, | ||
| _navigate: unknown, | ||
| options?: { search?: { terminalId?: string } }, | ||
| ) => { | ||
| navigated.push({ | ||
| workspaceId, | ||
| terminalId: options?.search?.terminalId, | ||
| }); | ||
| return Promise.resolve(); | ||
| }, | ||
| }), | ||
| ); | ||
| mock.module( | ||
| "renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/AgentIcon", | ||
| () => ({ AgentIcon: () => null }), | ||
| ); | ||
|
|
||
| const { act, cleanup, fireEvent, render, within } = await import( | ||
| "@testing-library/react" | ||
| ); | ||
| const { PageWatcherMenu } = await import("./PageWatcherMenu"); | ||
|
|
||
| afterEach(cleanup); | ||
| afterAll(async () => { | ||
| if (!alreadyRegistered) await GlobalRegistrator.unregister(); | ||
| }); | ||
|
|
||
| beforeEach(() => { | ||
| rows = []; | ||
| cloudWatch = { watching: false, agentId: null }; | ||
| navigated = []; | ||
| }); | ||
|
|
||
| function watcher(overrides: Partial<Row> = {}): Row { | ||
| return { | ||
| workspaceId: "ws-a", | ||
| workspaceName: "Chat UI", | ||
| terminalId: "term-1", | ||
| agentId: "codex", | ||
| sessionTitle: "Page watcher redesign", | ||
| hostId: "host-1", | ||
| ...overrides, | ||
| }; | ||
| } | ||
|
|
||
| async function renderMenu() { | ||
| let view!: ReturnType<typeof render>; | ||
| await act(async () => { | ||
| view = render( | ||
| <PageWatcherMenu workspaceId={WORKSPACE_ID} pageId={PAGE_ID} />, | ||
| ); | ||
| }); | ||
| return within(view.baseElement as HTMLElement); | ||
| } | ||
|
|
||
| async function openMenu() { | ||
| const ui = await renderMenu(); | ||
| await act(async () => { | ||
| fireEvent.pointerDown( | ||
| ui.getByRole("button"), | ||
| new Event("pointerdown", { bubbles: true }), | ||
| ); | ||
| }); | ||
| return ui; | ||
| } | ||
|
|
||
| describe("a page nothing is watching", () => { | ||
| test("shows no control at all, rather than a menu saying so", async () => { | ||
| const ui = await renderMenu(); | ||
| expect(ui.queryByRole("button")).toBeNull(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("a page one agent is watching", () => { | ||
| beforeEach(() => { | ||
| rows = [watcher()]; | ||
| cloudWatch = { watching: true, agentId: "codex" }; | ||
| }); | ||
|
|
||
| test("names the session and the workspace it sits in", async () => { | ||
| const ui = await openMenu(); | ||
| expect(ui.getByText("Page watcher redesign")).toBeDefined(); | ||
| expect(ui.getByText("Chat UI")).toBeDefined(); | ||
| }); | ||
|
|
||
| test("does not put the agent's name on the trigger", async () => { | ||
| const ui = await renderMenu(); | ||
| expect(ui.getByRole("button").textContent).not.toContain("codex"); | ||
| }); | ||
|
|
||
| test("opens that agent's terminal in its own workspace", async () => { | ||
| const ui = await openMenu(); | ||
| await act(async () => { | ||
| fireEvent.click(ui.getByText("Page watcher redesign")); | ||
| }); | ||
| expect(navigated).toEqual([{ workspaceId: "ws-a", terminalId: "term-1" }]); | ||
| }); | ||
|
|
||
| test("falls back to the agent's name when the session has no title", async () => { | ||
| rows = [watcher({ sessionTitle: null })]; | ||
| const ui = await openMenu(); | ||
| expect(ui.getByText("codex")).toBeDefined(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("a page several agents are watching", () => { | ||
| beforeEach(() => { | ||
| rows = [ | ||
| watcher(), | ||
| watcher({ | ||
| workspaceId: "ws-b", | ||
| workspaceName: "Onboarding flow", | ||
| terminalId: "term-2", | ||
| agentId: "claude", | ||
| sessionTitle: "Onboarding copy pass", | ||
| hostId: "host-2", | ||
| }), | ||
| ]; | ||
| cloudWatch = { watching: true, agentId: "codex" }; | ||
| }); | ||
|
|
||
| test("counts them on the trigger", async () => { | ||
| const ui = await renderMenu(); | ||
| expect(ui.getByRole("button").textContent).toContain("2"); | ||
| }); | ||
|
|
||
| test("lists every one of them", async () => { | ||
| const ui = await openMenu(); | ||
| expect(ui.getByText("Page watcher redesign")).toBeDefined(); | ||
| expect(ui.getByText("Onboarding copy pass")).toBeDefined(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("a watcher on a host this machine cannot reach", () => { | ||
| beforeEach(() => { | ||
| cloudWatch = { watching: true, agentId: "codex" }; | ||
| }); | ||
|
|
||
| test("still shows the badge, because comments do reach it", async () => { | ||
| const ui = await renderMenu(); | ||
| expect(ui.getByRole("button")).toBeDefined(); | ||
| }); | ||
|
|
||
| test("says where it is instead of pretending nothing watches", async () => { | ||
| const ui = await openMenu(); | ||
| expect(ui.getByText("On a host you can't reach")).toBeDefined(); | ||
| expect(ui.getByText("codex")).toBeDefined(); | ||
| }); | ||
|
|
||
| test("names it generically when the flag carries no agent", async () => { | ||
| cloudWatch = { watching: true, agentId: null }; | ||
| const ui = await openMenu(); | ||
| expect(ui.getByText("An agent")).toBeDefined(); | ||
| }); | ||
| }); |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Expose whether host queries have settled.
The hook returns only rows. A caller cannot tell "no watcher found" apart from "host queries still loading".
PageWatcherMenuuseswatchers.length === 0to decidewatchedElsewhere. This case happens when a watcher runs locally, the cloud flag sayswatching, and the host query has not resolved yet (first open, or a new host target). The menu then shows "On a host you can't reach" for a local watcher, and the label switches once the data arrives.Return a settled flag with the rows. Use
result.isPendingfrom the queries for this flag.Proposed change
In
PageWatcherMenu.tsx:📝 Committable suggestion
🤖 Prompt for AI Agents