You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "M4n5ter (via GitHub)" <gi...@apache.org> on 2026/08/24 15:58:00 UTC
[PR] fix(desktop): own Settings Escape before paint [maka]
M4n5ter opened a new pull request, #3730:
URL: https://github.com/apache/maka/pull/3730
<details open>
<summary>English</summary>
## Summary
Settings now installs its Escape owner during the layout phase, so the first frame in which the Settings surface is observable can already consume Escape.
The listener remains stable across shell re-renders and reads the current close callback from a ref. It preserves the existing precedence rule: a nested dialog that already prevented the event closes itself without also closing Settings.
Fixes #3728
## Root cause
The previous `useHotkeys` subscription attached its global listener from a passive effect. React could commit and paint the Settings DOM before that effect ran, leaving a brief interval where Settings was visible but the first Escape had no owner. The existing Electron E2E could reach that interval intermittently.
## 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 — 62 passed, 1 skipped
- The targeted titlebar-rename/Settings test passed 25 consecutive runs across stress and final-head verification
- `npx biome check apps/desktop/src/renderer/settings/settings-modal.tsx`
- `git diff --check`
## AI use
Select exactly one:
- [ ] No generative tool made a substantive contribution
- [x] Generative tooling made a substantive contribution
Tool(s) and scope: Codex implemented and verified the event-lifecycle fix. The affected commit includes a `Generated-by: Codex` trailer.
## Checklist
- [x] Tests cover the change and fail without it
- [x] Lint, format, typecheck and the affected suites pass locally
Does this PR entail a change in behavior?
- [x] Yes — Settings owns Escape from its first observable frame
- [ ] No
</details>
<details>
<summary>简体中文</summary>
## 摘要
Settings 现在会在布局阶段安装 Escape 的处理者，因此从 Settings 界面首次可观察的那一帧开始，第一次 Escape 就已有明确的处理者。
监听器在 shell 重渲染期间保持稳定，并通过 ref 读取当前的关闭回调。现有的嵌套优先级保持不变：如果内层对话框已经阻止该事件，它只关闭自己，不会连带关闭 Settings。
修复 #3728
## 根因
原来的 `useHotkeys` 订阅在被动 effect 中安装全局监听器。React 可能先提交并绘制 Settings DOM，之后才运行该 effect，因而留下一个短暂空窗：Settings 已经可见，但第一次 Escape 还没有处理者。现有 Electron E2E 会间歇性地命中这个空窗。
## 验证
- `npm --workspace @maka/desktop run build:with-deps`
- `npm --workspace @maka/desktop run typecheck`
- `npm --workspace @maka/desktop test` — 1,436 条通过
- 完整 Desktop Electron E2E — 62 条通过，1 条跳过
- 标题栏重命名/Settings 定向测试在压力与最终 head 验证中连续通过 25 次
- `npx biome check apps/desktop/src/renderer/settings/settings-modal.tsx`
- `git diff --check`
## AI 使用
- [ ] 没有生成式工具做出实质贡献
- [x] 生成式工具做出了实质贡献
工具与范围：Codex 实现并验证了事件生命周期修复。相关提交包含 `Generated-by: Codex` trailer。
## 检查清单
- [x] 测试覆盖该改动，并且在未修复时会失败
- [x] lint、format、typecheck 与受影响测试套件均在本地通过
此 PR 是否改变行为？
- [x] 是 — Settings 从首次可观察帧起即拥有 Escape
- [ ] 否
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): own Settings Escape before paint [maka]
Posted by "M4n5ter (via GitHub)" <gi...@apache.org>.
M4n5ter merged PR #3730:
URL: https://github.com/apache/maka/pull/3730
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): own Settings Escape before paint [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #3730:
URL: https://github.com/apache/maka/pull/3730#issuecomment-5398024211
<details open>
<summary>English</summary>
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.
</details>
<details>
<summary>简体中文</summary>
审查基于 `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 全部终态绿为前提。
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): own Settings Escape before paint [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #3730:
URL: https://github.com/apache/maka/pull/3730#issuecomment-5398707448
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.
<details>
<summary>简体中文</summary>
针对 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`；待其进入终态绿后再重新判断是否批准。
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org