Skip to content

fix: honour browser.toast=false when browser.native=true - #2

Open
fengfanfan-max wants to merge 1 commit into
c-ling:mainfrom
fengfanfan-max:fix/browser-toast-flag
Open

fengfanfan-max wants to merge 1 commit into
c-ling:mainfrom
fengfanfan-max:fix/browser-toast-flag

Conversation

@fengfanfan-max

Copy link
Copy Markdown

What is broken

「浏览器通知」和「系统原生通知」这两个开关并不独立。只要「系统原生通知」开着,把「浏览器通知」关掉,页面内的文字横幅照样会弹

Reproduction

browser:
  enabled: true
  toast: false     # ← 关掉页内横幅
  native: true     # ← 只保留系统原生通知

让页面保持可见,然后等一轮对话结束(或切走再回来触发一次 turn/end)。

Expected: 只弹系统原生通知,页面内没有横幅。
Actual: 页面内横幅也弹出来了。

只有两个开关都关,横幅才真的不出现。

Root cause

引擎里那两个变量的注释声称二者独立:

// The two browser-channel settings are independent: the in-page banner
// needs the 浏览器通知 master switch, the native OS notification only
// needs its own 系统原生通知 switch.
var showToast  = cfg.browser?.enabled !== false && cfg.browser?.toast !== false;
var showNative = cfg.browser?.native === true;
if (!showToast && !showNative) return;

showToast 参与了那个提前 return,从来没被传下去;而 fireBrowserNotification 里推横幅是无条件的:

function fireBrowserNotification(message, toastStore, options) {
  if (options && options.native === true) fireNativeNotification(message.title, message.body, options.tag);
  if (typeof document !== "undefined" && document.visibilityState === "hidden") return;
  if (toastStore === undefined) return;
  toastStore.push(message.title, message.body);   // ← 无条件
}

所以 native: truetoast: false 完全失效。

The fix

toast 传下去,并在 fireBrowserNotification 里短路,恢复注释已经承诺的独立性。设置页里那两个测试按钮仍以无 options 的方式调用,因此继续会弹横幅——那正是它们的用途。

Tests

改了 3 行,没有带测试lib/client.js 目前在 node --test 下零覆盖,要驱动它需要一个 DOM(bundle 加载过程中会用到 document.querySelector 等),仓库里没有 jsdom 之类的 devDependency。为一个三行的布尔短路把测试框架引进来,我觉得不该塞进这个 PR。

如果你希望带上测试,我可以单独提一个 PR 加上客户端测试骨架(jsdom + 桩 window.__ModuleLoader__),然后在这个 PR 里补上行为断言。你说一声就行。

验证

已在真实环境复现:{ enabled: true, toast: false, native: true } 下轮次结束时页内横幅确实出现。

The engine reads the two browser settings as independent switches:

    var showToast  = cfg.browser?.enabled !== false && cfg.browser?.toast !== false;
    var showNative = cfg.browser?.native === true;
    if (!showToast && !showNative) return;

but `showToast` was only ever used for that early return — it was never passed
on, and `fireBrowserNotification` pushed to the banner store unconditionally:

    function fireBrowserNotification(message, toastStore, options) {
      if (options && options.native === true) fireNativeNotification(...);
      if (document.visibilityState === "hidden") return;
      if (toastStore === undefined) return;
      toastStore.push(message.title, message.body);   // always
    }

So disabling "浏览器通知" while "系统原生通知" stayed on still produced the
in-page banner. Only turning BOTH off suppressed it.

Reproduction: with browser = { enabled: true, toast: false, native: true },
end a turn while the page is visible — a banner appears even though the banner
switch is off.

Pass the flag through and short-circuit on it, restoring the independence the
comment on those two variables already promises. The settings page's own test
buttons still call fireBrowserNotification without options, so they keep showing
a banner, which is what they are for.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant