Skip to content

fix(desktop): own Settings Escape before paint - #3730

Merged
M4n5ter merged 3 commits into
mainfrom
fix/3728-settings-escape-race
Aug 24, 2026
Merged

M4n5ter merged 3 commits into
mainfrom
fix/3728-settings-escape-race

Conversation

@M4n5ter

@M4n5ter M4n5ter commented Aug 24, 2026 •

Copy link
Copy Markdown
Member
English

Summary

The Settings overlay now installs its Escape owner during the layout phase, above the lazy-loaded Settings component. The same owner therefore covers both the visible Suspense loading surface and the resolved Settings surface from their first observable frame.

The listener remains stable across shell re-renders and reads the current close callback from a ref. It preserves the existing keyboard contract: nested layers that already prevented Escape keep priority, Ctrl/Cmd/Alt+Escape remain untouched, and unmodified or Shift+Escape close Settings even from an input.

Fixes #3728

Root cause

The previous useHotkeys subscription lived inside the lazy Settings component and attached its global listener from a passive effect. This left two intervals with no Escape owner:

  1. the visible Suspense fallback while the Settings chunk was still loading; and
  2. the interval after the resolved Settings DOM committed but before its passive effect ran.

The existing titlebar-rename E2E waited for the resolved Settings main region before pressing Escape, so it could expose the second race intermittently but could not exercise the loading fallback at all.

Verification

  • npm --workspace @maka/desktop run build:with-deps
  • npm --workspace @maka/desktop run typecheck
  • npm --workspace @maka/desktop test — 1,436 passed
  • Full Desktop Electron E2E suite — 64 passed, 1 skipped
  • Full Settings E2E spec — 6 passed
  • A new deterministic E2E latch holds the Settings chunk while the loading surface is visible; Ctrl/Cmd/Alt+Escape keep it open and one unmodified Escape closes it
  • The targeted titlebar-rename/Settings test passed 25 consecutive runs across stress and final-head verification
  • npx biome check on all changed source and E2E files
  • git diff --check

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Codex implemented and verified the event-lifecycle fix. The affected commits include a Generated-by: Codex trailer.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — Settings owns Escape from its first observable loading or resolved frame
  • No
简体中文

摘要

Settings overlay 现在会在布局阶段、lazy Settings 组件之外安装 Escape 的唯一处理者。因此从可见的 Suspense 加载界面到解析完成的 Settings 界面,两段都从各自首次可观察的帧开始拥有同一个处理者。

监听器在 shell 重渲染期间保持稳定,并通过 ref 读取当前的关闭回调。现有键盘契约保持不变:已经阻止 Escape 的内层界面优先处理;Ctrl/Cmd/Alt+Escape 不受影响;裸 Escape 或 Shift+Escape 即使在输入框中也会关闭 Settings。

修复 #3728

根因

原来的 useHotkeys 订阅位于 lazy Settings 组件内部,并在被动 effect 中安装全局监听器。这会留下两个没有 Escape 处理者的时间窗:

  1. Settings chunk 仍在加载时已经可见的 Suspense fallback;
  2. 真正的 Settings DOM 已提交、但被动 effect 尚未运行的时间窗。

原有标题栏重命名 E2E 会先等待解析完成的 Settings main 区域再按 Escape,因此它可能间歇性暴露第二个竞态,但完全无法覆盖 loading fallback。

验证

  • npm --workspace @maka/desktop run build:with-deps
  • npm --workspace @maka/desktop run typecheck
  • npm --workspace @maka/desktop test — 1,436 条通过
  • 完整 Desktop Electron E2E — 64 条通过,1 条跳过
  • 完整 Settings E2E — 6 条通过
  • 新增确定性 E2E latch:在 loading 界面可见时挂住 Settings chunk,验证 Ctrl/Cmd/Alt+Escape 保持打开,单次裸 Escape 关闭
  • 标题栏重命名/Settings 定向测试在压力与最终 head 验证中连续通过 25 次
  • 对所有变更源码和 E2E 文件执行 npx biome check
  • git diff --check

AI 使用

  • 没有生成式工具做出实质贡献
  • 生成式工具做出了实质贡献

工具与范围:Codex 实现并验证了事件生命周期修复。相关提交包含 Generated-by: Codex trailer。

检查清单

  • 测试覆盖该改动,并且在未修复时会失败
  • lint、format、typecheck 与受影响测试套件均在本地通过

此 PR 是否改变行为?

  • 是 — Settings 从首次可观察的 loading 或解析完成帧起即拥有 Escape
  • 否

Install the Settings dismissal listener during the layout phase so the first visible frame already has an Escape owner. Keep the listener stable across streamed shell renders while preserving nested-dialog precedence.

Generated-by: Codex
@Astro-Han

Copy link
Copy Markdown
Contributor
English

Reviewed at 174b68b31241cdb8b0fc31a24bf989516d2f9afe.

The listener lifetime is right. The ref refresh runs in the layout phase on every render with no dependency array, so a streaming re-render can never leave the handler holding a stale onClose; the subscription effect has an empty dependency array and its cleanup pairs with the registration, so a ref update never re-subscribes. Nested-dialog precedence also holds in the steady state — the Astryx <dialog> calls preventDefault() at the target node during bubbling and the focus trap prevents and stops at document, both ahead of window.

Two things are worth fixing before this merges.

1. The first observable Settings frame still has no Escape owner

app-shell-overlays.tsx loads SettingsModal through lazy() behind a Suspense boundary, and SettingsModalFallback is not an invisible placeholder — it is a full-screen settingsModal settingsPage surface with a spinner and data-surface="modal". By the time it is committed, settingsOpen has already torn down the titlebar identity and masked the shell, so the user sees the Settings transition. The real component has not mounted yet, so neither layout effect in settings-modal.tsx exists and Escape is dropped.

That is exactly the failure this change sets out to remove. The PR states that Settings owns Escape from its first observable frame; on a cold first open the first observable frame is the fallback, and a slow chunk load widens that window arbitrarily. The reproduction shape is unchanged from #3728: the Settings transition is visible, one Escape does nothing, and the titlebar identity is still gone.

Note this is not a regression — the previous useHotkeys call lived in the same component and had the same blind spot. What blocks here is that the stated behavior change is not delivered.

The existing coverage cannot reach it. settings.spec.ts:53 waits for the resolved modal's main (设置内容) to be visible before pressing Escape, so the fallback is never the target and 25 consecutive passes say nothing about this window.

Suggested direction: hoist the layout-phase owner to the Settings overlay / Suspense boundary and remove it from the lazy child, so there is exactly one close authority across the fallback → real handoff. For acceptance, deliberately stall the Settings chunk, press Escape once while the loading surface is visible, and assert it closes.

2. Modified Escape combinations now close Settings

The handler matches on event.key !== 'Escape' alone. The useHotkeys it replaces required Ctrl, Meta and Alt to all be unpressed for an escape combo — its matchesCombo returns early on event.ctrlKey !== wantsCtrl, event.metaKey !== wantsMeta and event.altKey !== wantsAlt, with Shift ignored unless declared. So Ctrl+Escape, Cmd+Escape and Alt+Escape change from "Settings ignores it" to "Settings closes".

These combinations are usually claimed by the OS, so preventDefault() will not suppress the system action — but the close callback still runs. What a user observes is a system shortcut that unexpectedly dismisses the Settings panel. The 1,436 unit tests and the E2E suite all dispatch bare Escape, so nothing currently guards this.

Either restore the old matching with if (event.ctrlKey || event.metaKey || event.altKey) return; at the top of the handler (leaving Shift through), or state the widened scope in the PR description as intended.

Checked and fine

preventDefault() on a bare Escape is not new — the replaced hook called event.preventDefault() on every match. Neither version filters IME composition (isComposing / keyCode 229) or event.repeat, so those are unchanged rather than missing. The old call passed allowInInputs: true, so the new handler ignoring the focus target is equivalent for this surface.

Approval is still pending terminal-green hosted checks on the exact head.

简体中文

审查基于 174b68b31241cdb8b0fc31a24bf989516d2f9afe。

监听器的生命周期没有问题。ref 刷新在布局阶段、每次渲染后执行且无依赖数组,因此流式重渲染不会让 handler 持有过期的 onClose;订阅 effect 依赖数组为空,cleanup 与注册成对,ref 更新不会导致重新订阅。稳态下的嵌套 dialog 优先级也成立——Astryx 的 <dialog> 在目标节点冒泡时调用 preventDefault(),focus trap 在 document 上 prevent 并 stop,两者都先于 window。

合并前有两处需要处理。

一、Settings 首个可观察帧仍然没有 Escape 的处理者

app-shell-overlays.tsx 通过 lazy() 在 Suspense 边界后加载 SettingsModal,而 SettingsModalFallback 并不是不可见的占位符——它是一个带 spinner、data-surface="modal" 的全屏 settingsModal settingsPage 界面。它被提交时,settingsOpen 已经卸载了标题栏 identity 并遮蔽了 shell,用户已经看到 Settings 的过渡。此时真正的组件尚未挂载,settings-modal.tsx 中的两个 layout effect 都不存在,Escape 被直接丢弃。

这正是本次改动要消除的失败。PR 声明 Settings 从首个可观察帧起就拥有 Escape;但冷启动首次打开时,首个可观察帧就是 fallback,chunk 加载慢会把这个窗口任意放大。复现形态与 #3728 一致:Settings 过渡已可见、按一次 Escape 没有反应、标题栏 identity 仍然缺失。

需要说明这不是回归——原来的 useHotkeys 调用位于同一个组件,有同样的盲区。这里之所以阻塞,是因为 PR 声明的行为变更没有真正兑现。

现有验证覆盖不到它。settings.spec.ts:53 会先等待已解析 modal 的 main(设置内容)可见,然后才按 Escape,所以 fallback 永远不是目标,连续 25 次通过对这个窗口不构成任何证据。

建议方向:把布局阶段的处理者上提到 Settings overlay / Suspense 边界,并从 lazy 子组件中移除,使 fallback → 真实组件的交接过程中始终只有一个关闭 authority。验收方式:故意挂住 Settings 的 chunk,在 loading 界面可见时按一次 Escape,断言其关闭。

二、带修饰键的 Escape 现在会关闭 Settings

新 handler 仅以 event.key !== 'Escape' 判断。被它替换的 useHotkeys 对 escape 组合要求 Ctrl、Meta、Alt 全部未按下——其 matchesCombo 在 event.ctrlKey !== wantsCtrl、event.metaKey !== wantsMeta、event.altKey !== wantsAlt 处提前返回,Shift 未声明时不校验。因此 Ctrl+Escape、Cmd+Escape、Alt+Escape 从「Settings 不理会」变成了「Settings 关闭」。

这些组合通常被操作系统占用,preventDefault() 并不能抑制系统行为——但关闭回调照样执行。用户观察到的现象是:按了一个系统快捷键,Settings 面板意外被关掉。1,436 个单测和 E2E 发送的都是不带修饰键的 Escape,目前没有任何东西能拦住它。

要么在 handler 开头恢复旧的匹配语义 if (event.ctrlKey || event.metaKey || event.altKey) return;(放行 Shift),要么在 PR 描述中声明这一范围扩大是有意为之。

已核对、没有问题的部分

裸 Escape 上调用 preventDefault() 并非新增行为——被替换的 hook 在每次匹配时都会调用 event.preventDefault()。两个版本都不过滤输入法组字(isComposing / keyCode 229)和 event.repeat,因此这些是保持一致而非缺失。旧调用传了 allowInInputs: true,所以新 handler 完全不看焦点目标,对这个界面而言是等价的。

批准仍以该 exact head 的 hosted checks 全部终态绿为前提。

@Astro-Han

Copy link
Copy Markdown
Contributor

Update for exact head df1e49a4dbb88428c0fb898f8b40f8bbc09c625f:

I rechecked the earlier concerns against this head. The Suspense fallback Escape-owner issue and the modifier-key Escape issue are both closed. The Escape owner is now installed by AppShellOverlays before paint while the fallback is visible, and the replacement handler preserves the previous behavior for Ctrl/Meta/Alt, Shift, input targets, already-prevented events, and nested dialogs.

Two independent rechecks found no new P0–P3 issue. The PR remains unapproved only because the required exact-head CI run 32754348295 is still in_progress; I will reassess approval after it reaches a terminal green state.

简体中文

针对 exact head df1e49a4dbb88428c0fb898f8b40f8bbc09c625f 更新:

我已在当前 head 上重新核对之前的疑点。Suspense fallback 阶段没有 Escape owner 的问题,以及修饰键 Escape 的问题,均已闭合。现在由 AppShellOverlays 在 fallback 可见时、首帧绘制前安装 Escape owner;替换后的 handler 保留了 Ctrl/Meta/Alt、Shift、输入目标、已 prevent 的事件以及嵌套 dialog 的原有行为。

两条独立复核线均未发现新的 P0–P3 问题。当前仍未批准的唯一原因是 required exact-head CI run 32754348295 仍处于 in_progress;待其进入终态绿后再重新判断是否批准。

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rechecked the current head df1e49a. The previously reported fallback and modifier-key issues are resolved, and I found no new blocking issue in the current diff. The exact-head CI run 32754348295 completed successfully.

@M4n5ter
M4n5ter merged commit 958176a into main Aug 24, 2026
1 check passed
@M4n5ter
M4n5ter deleted the fix/3728-settings-escape-race branch August 24, 2026 17:36
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.

test(desktop): eliminate the Settings/titlebar rename Escape race

2 participants