mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-12 22:17:00 +00:00
fix(browser): prevent stale handles from switching duplicate-URL tabs (#103816)
* fix(browser): reject ambiguous tab replacements * fix(browser): guide stale tab recovery
This commit is contained in:
committed by
GitHub
parent
97bbbc2271
commit
845311cf97
@@ -281,9 +281,10 @@ Notes:
|
||||
- `upload` can also set file inputs directly via `--input-ref` or `--element`.
|
||||
|
||||
Stable tab ids and labels survive Chromium raw-target replacement when OpenClaw
|
||||
can prove the replacement tab, such as same URL or a single old tab becoming a
|
||||
single new tab after form submission. Raw target ids are still volatile; prefer
|
||||
`suggestedTargetId` from `tabs` in scripts.
|
||||
can prove the replacement tab, such as a unique old/new pair for the same URL or
|
||||
a single old tab becoming a single new tab after form submission. Ambiguous
|
||||
duplicate-URL replacements receive fresh handles. Raw target ids are still
|
||||
volatile; prefer `suggestedTargetId` from `tabs` in scripts.
|
||||
|
||||
Snapshot flags at a glance:
|
||||
|
||||
|
||||
@@ -189,6 +189,47 @@ describe("browser remote profile tab ops via Playwright", () => {
|
||||
expect(state.profiles.get("remote")?.lastTargetId).toBe("A");
|
||||
});
|
||||
|
||||
it("migrates only unique URL groups alongside ambiguous duplicate groups", async () => {
|
||||
let currentPages = [
|
||||
page("A", "https://unique.example"),
|
||||
page("B", "https://duplicate.example"),
|
||||
page("C", "https://duplicate.example"),
|
||||
];
|
||||
const listPagesViaPlaywright = vi.fn(async () => currentPages);
|
||||
|
||||
vi.spyOn(deps.pwAiModule, "getPwAiModule").mockResolvedValue({
|
||||
listPagesViaPlaywright,
|
||||
} as unknown as Awaited<ReturnType<typeof deps.pwAiModule.getPwAiModule>>);
|
||||
|
||||
const { state, remote } = deps.createRemoteRouteHarness();
|
||||
|
||||
expect((await remote.listTabs()).map((tab) => [tab.targetId, tab.tabId])).toEqual([
|
||||
["A", "t1"],
|
||||
["B", "t2"],
|
||||
["C", "t3"],
|
||||
]);
|
||||
await remote.labelTab("t1", "unique");
|
||||
state.profiles.get("remote")!.lastTargetId = "A";
|
||||
|
||||
currentPages = [
|
||||
page("D", "https://unique.example"),
|
||||
page("E", "https://duplicate.example"),
|
||||
page("F", "https://duplicate.example"),
|
||||
];
|
||||
|
||||
await expect(remote.listTabs()).resolves.toEqual([
|
||||
expect.objectContaining({
|
||||
targetId: "D",
|
||||
tabId: "t1",
|
||||
label: "unique",
|
||||
suggestedTargetId: "unique",
|
||||
}),
|
||||
expect.objectContaining({ targetId: "E", tabId: "t4", suggestedTargetId: "t4" }),
|
||||
expect.objectContaining({ targetId: "F", tabId: "t5", suggestedTargetId: "t5" }),
|
||||
]);
|
||||
expect(state.profiles.get("remote")?.lastTargetId).toBe("D");
|
||||
});
|
||||
|
||||
it("prefers lastTargetId for remote profiles when targetId is omitted", async () => {
|
||||
const responses = [
|
||||
[
|
||||
|
||||
@@ -199,6 +199,15 @@ describe("browser profile tab selection", () => {
|
||||
await expect(selection.ensureTabAvailable()).resolves.toEqual(sticky);
|
||||
});
|
||||
|
||||
it("rejects when a sticky target disappears instead of selecting another tab", async () => {
|
||||
const other = tab("OTHER", "ws://127.0.0.1/devtools/page/OTHER");
|
||||
const { selection, profileState } = createSelectionHarness({ snapshots: [[other]] });
|
||||
profileState.lastTargetId = "STALE";
|
||||
|
||||
await expect(selection.ensureTabAvailable()).rejects.toThrow(/use action=tabs/i);
|
||||
expect(profileState.lastTargetId).toBe("STALE");
|
||||
});
|
||||
|
||||
it("keeps polling after a transient tab-list rejection", async () => {
|
||||
vi.useFakeTimers();
|
||||
const withoutWs = tab("RECOVERED");
|
||||
|
||||
@@ -195,15 +195,16 @@ export function createProfileSelectionOps({
|
||||
return candidates.find((t) => t.targetId === resolved.targetId) ?? null;
|
||||
};
|
||||
|
||||
const stickyTargetId = normalizeOptionalString(profileState.lastTargetId);
|
||||
const pickDefault = () => {
|
||||
const last = normalizeOptionalString(profileState.lastTargetId) ?? "";
|
||||
const last = stickyTargetId ?? "";
|
||||
const lastResolved = last ? resolveById(last, { exactTargetId: true }) : null;
|
||||
if (lastResolved && lastResolved !== "AMBIGUOUS") {
|
||||
return lastResolved;
|
||||
}
|
||||
// Chrome MCP identity is authoritative. Once a selected target disappears,
|
||||
// require a fresh explicit choice instead of guessing another tab.
|
||||
if (last && capabilities.usesChromeMcp) {
|
||||
// Sticky selection is an identity promise. If it disappears without a proven
|
||||
// alias migration, require a fresh explicit choice instead of guessing a tab.
|
||||
if (last) {
|
||||
return null;
|
||||
}
|
||||
// Prefer a real page tab first (avoid service workers/background targets).
|
||||
@@ -217,7 +218,7 @@ export function createProfileSelectionOps({
|
||||
throw new BrowserTargetAmbiguousError();
|
||||
}
|
||||
if (!chosen) {
|
||||
throw new BrowserTabNotFoundError(targetId ? { input: targetId } : undefined);
|
||||
throw new BrowserTabNotFoundError({ input: targetId ?? stickyTargetId });
|
||||
}
|
||||
profileState.lastTargetId = chosen.targetId;
|
||||
return chosen;
|
||||
|
||||
@@ -121,18 +121,33 @@ function assignTabAlias(params: {
|
||||
};
|
||||
}
|
||||
|
||||
function isConfidentReplacement(params: {
|
||||
staleEntry: { url?: string };
|
||||
tab: BrowserTab;
|
||||
staleCount: number;
|
||||
newCandidateCount: number;
|
||||
}): boolean {
|
||||
const staleUrl = params.staleEntry.url?.trim();
|
||||
const tabUrl = params.tab.url?.trim();
|
||||
if (staleUrl && tabUrl && staleUrl === tabUrl) {
|
||||
return true;
|
||||
type TabAliasEntry = NonNullable<ProfileRuntimeState["tabAliases"]>["byTargetId"][string];
|
||||
|
||||
function normalizeReplacementUrl(url: string | undefined): string | undefined {
|
||||
return url?.trim() || undefined;
|
||||
}
|
||||
|
||||
function findConfidentReplacement(params: {
|
||||
staleEntry: TabAliasEntry;
|
||||
staleEntries: Array<[targetId: string, entry: TabAliasEntry]>;
|
||||
newCandidates: BrowserTab[];
|
||||
}): BrowserTab | undefined {
|
||||
const { staleEntry, staleEntries, newCandidates } = params;
|
||||
// Preserve shipped form-submit continuity when the replacement set is one-for-one.
|
||||
if (staleEntries.length === 1 && newCandidates.length === 1) {
|
||||
return newCandidates[0];
|
||||
}
|
||||
return params.staleCount === 1 && params.newCandidateCount === 1;
|
||||
|
||||
const url = normalizeReplacementUrl(staleEntry.url);
|
||||
if (!url) {
|
||||
return undefined;
|
||||
}
|
||||
const staleMatches = staleEntries.filter(
|
||||
([, entry]) => normalizeReplacementUrl(entry.url) === url,
|
||||
);
|
||||
const candidates = newCandidates.filter((tab) => normalizeReplacementUrl(tab.url) === url);
|
||||
// Duplicate URL buckets have no ordering contract, so only migrate an exact 1:1 bucket.
|
||||
return staleMatches.length === 1 && candidates.length === 1 ? candidates[0] : undefined;
|
||||
}
|
||||
|
||||
function assignTabAliases(
|
||||
@@ -146,26 +161,14 @@ function assignTabAliases(
|
||||
([targetId]) => !liveTargetIds.has(targetId),
|
||||
);
|
||||
const newCandidates = tabs.filter((tab) => !aliases.byTargetId[tab.targetId]);
|
||||
const claimedTargetIds = new Set<string>();
|
||||
|
||||
if (migrateReplacements) {
|
||||
for (const [oldTargetId, staleEntry] of staleEntries) {
|
||||
const candidate = newCandidates.find(
|
||||
(tab) =>
|
||||
!claimedTargetIds.has(tab.targetId) &&
|
||||
isConfidentReplacement({
|
||||
staleEntry,
|
||||
tab,
|
||||
staleCount: staleEntries.length,
|
||||
newCandidateCount: newCandidates.length,
|
||||
}),
|
||||
);
|
||||
const candidate = findConfidentReplacement({ staleEntry, staleEntries, newCandidates });
|
||||
if (!candidate) {
|
||||
continue;
|
||||
}
|
||||
aliases.byTargetId[candidate.targetId] = staleEntry;
|
||||
delete aliases.byTargetId[oldTargetId];
|
||||
claimedTargetIds.add(candidate.targetId);
|
||||
if (profileState.lastTargetId === oldTargetId) {
|
||||
profileState.lastTargetId = candidate.targetId;
|
||||
}
|
||||
|
||||
@@ -565,6 +565,71 @@ describe("browser server-context tab selection state", () => {
|
||||
expect(state.profiles.get("openclaw")?.lastTargetId).toBe("NEW_RAW");
|
||||
});
|
||||
|
||||
it("expires aliases when duplicate-URL targets are replaced ambiguously", async () => {
|
||||
let targets = [
|
||||
{ id: "OLD_LEFT", title: "Left", url: "https://app.example/same" },
|
||||
{ id: "OLD_RIGHT", title: "Right", url: "https://app.example/same" },
|
||||
];
|
||||
const fetchMock = vi.fn(async (url: unknown) => {
|
||||
const value = String(url);
|
||||
if (!value.includes("/json/list")) {
|
||||
throw new Error(`unexpected fetch: ${value}`);
|
||||
}
|
||||
return {
|
||||
ok: true,
|
||||
json: async () =>
|
||||
targets.map((target) => ({
|
||||
id: target.id,
|
||||
title: target.title,
|
||||
url: target.url,
|
||||
webSocketDebuggerUrl: `ws://127.0.0.1/devtools/page/${target.id}`,
|
||||
type: "page",
|
||||
})),
|
||||
} as unknown as Response;
|
||||
});
|
||||
|
||||
global.fetch = withBrowserFetchPreconnect(fetchMock);
|
||||
const state = makeState("openclaw");
|
||||
const ctx = createTestBrowserRouteContext({ getState: () => state });
|
||||
const openclaw = ctx.forProfile("openclaw");
|
||||
|
||||
expect((await openclaw.listTabs()).map((tab) => [tab.targetId, tab.tabId])).toEqual([
|
||||
["OLD_LEFT", "t1"],
|
||||
["OLD_RIGHT", "t2"],
|
||||
]);
|
||||
await openclaw.labelTab("t1", "left");
|
||||
await openclaw.labelTab("t2", "right");
|
||||
state.profiles.get("openclaw")!.lastTargetId = "OLD_LEFT";
|
||||
|
||||
targets = [
|
||||
{ id: "NEW_RIGHT", title: "Right", url: "https://app.example/same" },
|
||||
{ id: "NEW_LEFT", title: "Left", url: "https://app.example/same" },
|
||||
];
|
||||
|
||||
await expect(openclaw.listTabs()).resolves.toEqual([
|
||||
expect.objectContaining({ targetId: "NEW_RIGHT", tabId: "t3", suggestedTargetId: "t3" }),
|
||||
expect.objectContaining({ targetId: "NEW_LEFT", tabId: "t4", suggestedTargetId: "t4" }),
|
||||
]);
|
||||
expect(state.profiles.get("openclaw")?.lastTargetId).toBe("OLD_LEFT");
|
||||
await expect(openclaw.ensureTabAvailable("left")).rejects.toThrow(/tab not found/i);
|
||||
await expect(openclaw.ensureTabAvailable()).rejects.toThrow(/tab not found/i);
|
||||
|
||||
await openclaw.labelTab("t3", "fresh-right");
|
||||
targets = [
|
||||
{ id: "NEW_LEFT", title: "Left", url: "https://app.example/same" },
|
||||
{ id: "NEWER_RIGHT", title: "Right", url: "https://app.example/same" },
|
||||
];
|
||||
await expect(openclaw.listTabs()).resolves.toEqual([
|
||||
expect.objectContaining({ targetId: "NEW_LEFT", tabId: "t4" }),
|
||||
expect.objectContaining({
|
||||
targetId: "NEWER_RIGHT",
|
||||
tabId: "t3",
|
||||
label: "fresh-right",
|
||||
suggestedTargetId: "fresh-right",
|
||||
}),
|
||||
]);
|
||||
});
|
||||
|
||||
it("resolves friendly tab references before backend focus and close calls", async () => {
|
||||
const fetchMock = vi.fn(async (url: unknown) => {
|
||||
const value = String(url);
|
||||
|
||||
Reference in New Issue
Block a user