fix(scripting): make Framework.Key input gating robust (alt-tab + reconnect)#233
fix(scripting): make Framework.Key input gating robust (alt-tab + reconnect)#233JEssou115 wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughKeybind physical-state detection now requires the process to own the foreground window. Shutdown no longer clears the host-provided active callback, which remains externally managed across scripting sessions. ChangesKeybind behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
code/framework/src/integrations/client/scripting/builtins/keybinds.cpp (2)
430-435: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComment is written in French while the rest of the file is English.
This block documents a subtle, important lifetime contract (
_activeCallbackoutliving scripting sessions), but it's the only non-English comment in the file. Non-French-speaking maintainers won't be able to follow the rationale.📝 Suggested translation
- // NOTE: on NE réinitialise PAS _activeCallback ici. C'est un gate HOST (posé par le mod via - // SetActiveCallback) qui capture l'application, laquelle vit pour tout le process — bien au-delà - // d'une session scripting. L'effacer à chaque Shutdown (déconnexion) faisait perdre la - // suppression des keybinds après une RECONNEXION (le mod ne ré-installe le gate qu'au démarrage - // complet du jeu), d'où des hotkeys qui se déclenchaient à nouveau en tapant dans le chat. - // Le host peut toujours l'annuler explicitement via SetActiveCallback(nullptr) s'il le veut. + // NOTE: _activeCallback is intentionally NOT reset here. It's a HOST gate (set by the mod via + // SetActiveCallback) that captures application-level state living for the whole process — well + // beyond a single scripting session. Clearing it on every Shutdown (disconnect) used to lose the + // keybind suppression after a RECONNECT (the mod only re-installs the gate on full game startup), + // causing hotkeys to fire again while typing in chat. + // The host can still explicitly clear it via SetActiveCallback(nullptr) if desired.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/framework/src/integrations/client/scripting/builtins/keybinds.cpp` around lines 430 - 435, Translate the explanatory comment above _activeCallback into English, preserving its full lifetime contract: Shutdown must not reset the host-installed callback, it must survive scripting reconnections, and SetActiveCallback(nullptr) remains the explicit cancellation path.
117-127: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueForeground-ownership gating is correctly implemented; per-key syscall redundancy could be hoisted out of the loop.
The foreground-ownership check correctly rejects key state while backgrounded, including the edge case where
GetForegroundWindow()returnsNULL(the zero-initializedpidnever matches a real PID). One optimization opportunity:Update()callsIsPhysicallyDownonce per bucket, soGetForegroundWindow()/GetWindowThreadProcessId()are re-queried for every registered key every tick instead of once per tick.♻️ Optional: compute foreground ownership once per Update() tick
- bool IsPhysicallyDown(int vk) { + bool IsForegroundOwnedByUs() { + DWORD pid = 0; + ::GetWindowThreadProcessId(::GetForegroundWindow(), &pid); + return pid == ::GetCurrentProcessId(); + } + + bool IsPhysicallyDown(int vk, bool foregroundOwned = true) { if ((::GetAsyncKeyState(vk) & 0x8000) == 0) { return false; } - // GetAsyncKeyState is a GLOBAL key state — it reads the key even while our window is in the - // background (alt-tab). Honor the documented "false while backgrounded" by requiring our own - // process to own the foreground window. - DWORD pid = 0; - ::GetWindowThreadProcessId(::GetForegroundWindow(), &pid); - return pid == ::GetCurrentProcessId(); + return foregroundOwned; }Then in
Update(), computeconst bool foregroundOwned = IsForegroundOwnedByUs();once before the bucket loop and pass it through.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/framework/src/integrations/client/scripting/builtins/keybinds.cpp` around lines 117 - 127, Hoist the foreground-window ownership check out of the per-key IsPhysicallyDown path: add or reuse an IsForegroundOwnedByUs helper, compute its result once at the start of Update() before the bucket loop, and pass that boolean into each key-state check while preserving false behavior when the foreground window is unavailable or belongs to another process.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@code/framework/src/integrations/client/scripting/builtins/keybinds.cpp`:
- Around line 430-435: Translate the explanatory comment above _activeCallback
into English, preserving its full lifetime contract: Shutdown must not reset the
host-installed callback, it must survive scripting reconnections, and
SetActiveCallback(nullptr) remains the explicit cancellation path.
- Around line 117-127: Hoist the foreground-window ownership check out of the
per-key IsPhysicallyDown path: add or reuse an IsForegroundOwnedByUs helper,
compute its result once at the start of Update() before the bucket loop, and
pass that boolean into each key-state check while preserving false behavior when
the foreground window is unavailable or belongs to another process.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 81fb75e8-84fe-492b-bdb5-5ac610ab6418
📒 Files selected for processing (1)
code/framework/src/integrations/client/scripting/builtins/keybinds.cpp
|
I can't read french :( |
|
Comments should actually be removed, its AI cancer comments. Code is quite self explanatory |
Two issues let a bound key fire when it should have been suppressed: 1. Backgrounded window (alt-tab). IsPhysicallyDown used GetAsyncKeyState, which reports a global key state — the key reads as down even when our window is not focused, so a key pressed in another application still fired the binding. Require our own process to own the foreground window, honoring the documented "false while the window is backgrounded" contract. 2. Reconnect. Keybinds::Shutdown() reset _activeCallback (the host input gate) on every scripting-engine teardown. The host installs that gate once and it captures host state that outlives scripting sessions, so after the first disconnect the gate was gone until a full process restart — and bound keys were no longer suppressed while a UI/chat view owned input (e.g. a letter typed into chat also toggled its binding). Keep the host gate across scripting shutdown; the host can still clear it explicitly via SetActiveCallback(nullptr). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
JEssou115
force-pushed
the
fix/framework-key-input-gate
branch
from
5d6e79a to
4610201
Compare
3 participants
What
Two independent robustness fixes to
Framework.Key(client keybinding) input gating, both inkeybinds.cpp.1. Don't read global key state while the window is backgrounded (alt-tab)
IsPhysicallyDownusedGetAsyncKeyState, which returns a global key state: a bound key reads as "down" even when our window isn't focused, so pressing that key in another application still fired the binding.Key.isDownis documented as false while the window is backgrounded, but the implementation didn't honor it.Fix: require our own process to own the foreground window (
GetWindowThreadProcessId(GetForegroundWindow()) == GetCurrentProcessId()).2. Keep the host input gate across a scripting-engine restart (reconnect)
The host registers an "active gate" once via
SetActiveCallback(...)— a predicate that suppresses key dispatch while a UI/chat view owns input. That callback captures host state which outlives scripting sessions. ButKeybinds::Shutdown()reset_activeCallbackon every scripting-engine teardown (i.e. on every server disconnect), so after the first disconnect the gate was gone until a full process restart, and on reconnect bound keys fired again while typing into chat.Fix: don't clear
_activeCallbackinShutdown()(still clear the per-session_bucketsand_resourceManager). The host can clear it explicitly withSetActiveCallback(nullptr)if needed.Why
Found while building a client mod that binds letter keys (e.g. a debug fly toggle). Symptoms:
Notes
keybinds.cppand backward compatible.#2only removes an over-eager reset; a host that relied on the gate being cleared on shutdown can callSetActiveCallback(nullptr)explicitly.🤖 Generated with Claude Code
Summary by CodeRabbit