diff --git a/entrypoints/background/grouping.test.ts b/entrypoints/background/grouping.test.ts index be98e77..cafeab2 100644 --- a/entrypoints/background/grouping.test.ts +++ b/entrypoints/background/grouping.test.ts @@ -7,6 +7,7 @@ import { createSerialQueue } from "../../utils/grouping/serial"; import type { Rule } from "../../utils/rules/types"; import { stub, useFakeTimersInTest } from "../../utils/testing/mocks"; import { onRulesChanged, onTabRemoved, onTitleChanged, regroupTab } from "./grouping"; +import { titlesStore } from "./rules"; const DEV: Rule = { id: "dev", name: "開発", color: "blue", conditions: [{ type: "contains", value: "/dev/" }] }; @@ -51,6 +52,29 @@ function mockDevTabWindow() { return { group, updateGroup }; } +/** ウィンドウ1にタブ10（/dev/を含むURL）があり、すでにグループ100（タイトル「開発」）に入っている状態にする */ +function mockDevTabInGroup() { + stub( + fakeBrowser.tabs, + "query", + vi + .fn<(queryInfo: Browser.tabs.QueryInfo) => Promise>() + .mockResolvedValue([fakeTab({ id: 10, url: "https://example.com/dev/1", groupId: 100, windowId: 1 })]), + ); + stub( + fakeBrowser.tabGroups, + "query", + vi + .fn<(queryInfo: Browser.tabGroups.QueryInfo) => Promise>() + .mockResolvedValue([{ id: 100, windowId: 1, title: "開発", color: "blue", collapsed: false, shared: false }]), + ); + const group = vi.fn<(options: Browser.tabs.GroupOptions) => Promise>(); + stub(fakeBrowser.tabs, "group", group); + const updateGroup = vi.fn<(groupId: number, properties: Browser.tabGroups.UpdateProperties) => Promise>(); + stub(fakeBrowser.tabGroups, "update", updateGroup); + return { group, updateGroup }; +} + /** rulesReader.read() が読む保存されたルールの一覧を設定する（本番の storage への保存に相当） */ async function setStoredRules(rules: readonly Rule[]): Promise { await fakeBrowser.storage.local.set({ rules: [...rules] }); @@ -203,3 +227,32 @@ describe("名前が衝突するルールの編集とタブイベントの重な expect(group).toHaveBeenCalledWith({ createProperties: { windowId: 1 }, tabIds: [10] }); }); }); + +describe("ルール名の打ち直し中のタブイベントとの競合（Issue #110）", () => { + it("反映前のタブイベントが記録を先に進めても、確定後に実際のグループを改名する", async () => { + // 前提: ルール「開発」はすでにグループ100（タイトル「開発」）を持っており、titlesStore にも記録済み。 + // このルールを「Dev」に改名する。ストレージの書き込み自体はすぐ終わるが、onRulesChanged + // （storage の watch 通知）が届くのは、別経路のタブイベントより後になることがある + // 検証: 反映タスクがキューへ積まれる前に、そのタブイベントが先に新しい名前「Dev」を読み、 + // titlesStore の記録だけを先に「Dev」へ進めてしまっても、通知が届いた後の反映は + // 実際のグループ100を「開発」から「Dev」へ改名する（先回りした記録のせいでスキップされない） + useFakeTimersInTest(); + fakeBrowser.reset(); + const { updateGroup } = mockDevTabInGroup(); + await setStoredRules([DEV]); + await titlesStore.write(new Map([["dev", "開発"]])); + const { onRulesChanged: onChange, onTabEvent, flush } = createHarness(); + + const renamed = { ...DEV, name: "Dev" }; + await setStoredRules([renamed]); + await onTabEvent(1, 10); + expect(updateGroup).not.toHaveBeenCalled(); + + // ここでようやく storage の watch 通知が届き、反映タスクがキューへ積まれる + onChange([renamed], [DEV]); + await vi.advanceTimersByTimeAsync(300); + await flush(); + + expect(updateGroup).toHaveBeenCalledWith(100, { title: "Dev", color: "blue" }); + }); +}); diff --git a/entrypoints/background/grouping.ts b/entrypoints/background/grouping.ts index 3b2d725..2782fce 100644 --- a/entrypoints/background/grouping.ts +++ b/entrypoints/background/grouping.ts @@ -4,7 +4,7 @@ import type { Browser } from "wxt/browser"; import { debounceChanges } from "../../utils/grouping/debounce"; import type { RulesState } from "../../utils/grouping/regroup"; import { applyRuleChange, regroupTabs } from "../../utils/grouping/regroup"; -import { currentTitles } from "../../utils/grouping/rule-change"; +import { titlesForNewRules } from "../../utils/grouping/rule-change"; import type { createSerialQueue } from "../../utils/grouping/serial"; import { logListenerErrors } from "../../utils/logging/listener"; import { getAppLogger } from "../../utils/logging/setup"; @@ -59,15 +59,17 @@ async function regroupAll(): Promise { * @param windowId - 対象のタブが属するウィンドウの ID * @param tabId - 判定し直す対象のタブの ID * @returns enqueue に積む処理 - * @remarks 判定に使ったルールの今の有効なタイトルを titlesStore にも記録する。これをしないと、このタブイベントが + * @remarks 判定に使ったルールのうち、まだ記録に無いものだけタイトルを titlesStore に足す。これをしないと、このタブイベントが * ルールの変更の反映より先にグループを作った場合、そのグループの存在が diffRules の記録から漏れ、 - * 後から確定する名前・色の変更がそのグループへ届かなくなる（Issue #36） + * 後から確定する名前・色の変更がそのグループへ届かなくなる（Issue #36）。 + * 記録済みのルールのタイトルは上書きしない。ここで名前の変更まで記録してしまうと、まだ実行していないルール変更の + * 反映（applyRuleChange）が「変更済み」と誤認し、実際のグループの改名を永久にスキップしてしまう（Issue #110） */ export function regroupTab(windowId: number, tabId: number): () => Promise { return async () => { const state = await readRulesState(); await regroupTabs(browserTabs, state, { windowId, tabIds: [tabId] }); - await titlesStore.write(currentTitles(state.rules, state.titles)); + await titlesStore.write(titlesForNewRules(state.rules, state.titles)); }; } diff --git a/utils/grouping/rule-change.ts b/utils/grouping/rule-change.ts index 16dc511..da45133 100644 --- a/utils/grouping/rule-change.ts +++ b/utils/grouping/rule-change.ts @@ -86,18 +86,27 @@ export function diffRules( } /** - * ルールの一覧自体は変わっていない読み直し（タブイベントの判定等）で、記録されているタイトルを - * 今の有効性に合わせ直す。 + * ルールの一覧自体は変わっていない読み直し（タブイベントの判定等）で、まだ記録に無いルールの分だけ + * タイトルを足す。 * * @param rules - 判定に使うルールの一覧 * @param titles - 現在記録されているタイトル - * @returns 有効なルールが今持つべきタイトル - * @remarks diffRules を oldRules と newRules が同じ呼び出しとして使うため、名前・色の変更（updates）は出ない。 - * タブイベントの素朴な判定（regroupTab）はこのタイトルの記録を更新しないため、後から来るルール変更の反映 - * （diffRules）が、タブイベントで先に作られたグループの存在を見落とさないよう、ここで記録を追いつかせる + * @returns 記録に無かった分だけタイトルを足した記録 + * @remarks 足すのは、タブイベントがルール変更の反映より先にグループを新規作成した場合に、そのグループの存在が + * diffRules の記録から漏れ、後から来るルール変更の反映がそのグループへ届かなくなるのを防ぐため（Issue #36）。 + * 記録済みのルールのタイトルは、ルールの今の名前と違っていても上書きしない。タブイベントの素朴な判定 + * （regroupTab）は実際の改名を行わないため、ここで記録だけ新しい名前に進めてしまうと、まだ実行していない + * ルール変更の反映（diffRules・applyRuleChange）が「すでに変更済み」と誤認し、実際のグループの改名を + * 永久にスキップしてしまう（Issue #110） */ -export function currentTitles(rules: readonly Rule[], titles: RuleTitles): RuleTitles { - return diffRules(rules, rules, titles).titles; +export function titlesForNewRules(rules: readonly Rule[], titles: RuleTitles): RuleTitles { + const result = new Map(titles); + for (const rule of validRules(rules, titles)) { + if (!result.has(rule.id)) { + result.set(rule.id, rule.name); + } + } + return result; } /** 有効なルールのグループのタイトル・色の変更。持っていたタイトルの変更を先に決め、変更されないタイトルを引き継いだルールは色だけを変える */ diff --git a/utils/grouping/titles-for-new-rules.test.ts b/utils/grouping/titles-for-new-rules.test.ts new file mode 100644 index 0000000..b7002fc --- /dev/null +++ b/utils/grouping/titles-for-new-rules.test.ts @@ -0,0 +1,41 @@ +import { describe, expect, it } from "vitest"; + +import type { Rule } from "../rules/types"; +import { titlesForNewRules } from "./rule-change"; + +const DEV: Rule = { id: "dev", name: "開発", color: "blue", conditions: [{ type: "contains", value: "github.com" }] }; +const DOCS: Rule = { + id: "docs", + name: "資料", + color: "green", + conditions: [{ type: "contains", value: "example.com" }], +}; + +/** id のルールが name のタイトルを持っている RuleTitles */ +function titlesOf(...entries: [Rule, string][]): Map { + return new Map(entries.map(([rule, title]) => [rule.id, title])); +} + +describe("タブイベントの判定し直しで足すタイトル（Issue #36・#110）", () => { + it("記録に無いルールのタイトルを足す", () => { + // 前提: DEV のタイトルがまだ記録に無い（タブイベントがルールの新規作成より先に判定した場合に相当） + // 検証: DEV のタイトルとして、今の名前「開発」が足される + expect(titlesForNewRules([DEV], new Map())).toStrictEqual(titlesOf([DEV, "開発"])); + }); + + it("記録済みのルールのタイトルは、今の名前と違っていても上書きしない", () => { + // 前提: DEV は記録上まだ「開発」のままだが、ルールの名前はすでに「Dev」に変わっている + // （ルール変更の反映がまだ終わっていない間に、タブイベントが判定し直した場合に相当） + // 検証: 記録は「開発」のまま変わらない。ここで「Dev」に進めてしまうと、まだ実行していないルール変更の + // 反映が「変更済み」と誤認し、実際のグループの改名が永久にスキップされてしまう（Issue #110） + expect(titlesForNewRules([{ ...DEV, name: "Dev" }], titlesOf([DEV, "開発"]))).toStrictEqual( + titlesOf([DEV, "開発"]), + ); + }); + + it("記録済みのルールと記録に無いルールが混ざっていても、記録済みの分だけ変えない", () => { + expect(titlesForNewRules([{ ...DEV, name: "Dev" }, DOCS], titlesOf([DEV, "開発"]))).toStrictEqual( + titlesOf([DEV, "開発"], [DOCS, "資料"]), + ); + }); +});