Record keypress gestures in Record Browser - #8481
Conversation
Sync PR: 16454 Sync SHA: 93913c443bc94e1437f3d4ba3aa7e01479aa92d4
Sync PR: 16454 Sync SHA: 93913c443bc94e1437f3d4ba3aa7e01479aa92d4
…src/' Sync PR: 16454 Sync SHA: 93913c443bc94e1437f3d4ba3aa7e01479aa92d4
Sync PR: 16454 Sync SHA: 93913c443bc94e1437f3d4ba3aa7e01479aa92d4
Sync PR: 16454 Sync SHA: 93913c443bc94e1437f3d4ba3aa7e01479aa92d4
Sync PR: 16454 Sync SHA: 93913c443bc94e1437f3d4ba3aa7e01479aa92d4
|
✅ Migration Auto-Generated An alembic migration was automatically generated from the source migration. Sync PR: 16454 |
| return false; | ||
| } | ||
| return ( | ||
| candidate.toLowerCase().includes("drive.google.com") || |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0bcb63a1f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sm.Select(), | ||
| # After InputText: an Enter that submits a field must record as the fill | ||
| # followed by the keypress, not replace it. | ||
| sm.PressKey(), |
There was a problem hiding this comment.
When two different keypresses target the same element within 250 ms, such as Ctrl+A followed by Ctrl+C, enabling this state machine sends both through _is_duplicate_action, but _action_identity does not include ActionPressKey.key; every press therefore has the same identity and the second gesture is discarded. Include the key expression in the identity so duplicate transport events still collapse without merging distinct shortcuts.
Useful? React with 👍 / 👎.
| # shifted in KeyboardEvent.key, and Shift+Tab / Shift+ArrowLeft are the same focus and | ||
| # selection noise as their bare forms. It does qualify a gesture already being recorded, | ||
| # where it changes what the key means -- Shift+Enter is a newline, not a submit. | ||
| shifted = modifiers.shift and not is_character_key(key) |
There was a problem hiding this comment.
Whenever a Ctrl/Alt/Meta shortcut also includes Shift and a character key, such as Ctrl+Shift+T, this sets shifted to false and records Control+T. Capitalizing KeyboardEvent.key does not preserve the modifier during replay—the repository's shortcut parser only holds modifiers explicitly listed before +—so the replay can perform a different action, such as opening a new tab instead of reopening one. Preserve Shift for retained modifier shortcuts while continuing to ignore Shift-only typing.
Useful? React with 👍 / 👎.
| # Alt composes characters -- macOS Option, and Windows/Linux AltGr, which reports as | ||
| # ctrl+alt. KeyboardEvent.key is then the composed output rather than the base key, so an | ||
| # Alt-held single character is typed text, not a shortcut. Layouts that need AltGr to reach | ||
| # "@" would otherwise lose every email field to a bogus Control+Alt+@ press. | ||
| if modifiers.alt and is_character_key(key): | ||
| return None |
There was a problem hiding this comment.
On pages that bind Alt+letter shortcuts, the browser reports a one-character key with altKey=true, so this condition drops the intended gesture entirely. AltGr or macOS Option composition is not equivalent to every Alt-modified character; capture composition/AltGraph information or the physical key code and suppress only composed input rather than all Alt+character shortcuts.
Useful? React with 👍 / 👎.
| key = (key or "").strip() | ||
| if not key or key in MODIFIER_KEYS: | ||
| return None |
There was a problem hiding this comment.
For Ctrl+Space, Meta+Space, or Alt+Space, KeyboardEvent.key is a literal space, and stripping it here produces an empty string before modifier processing. These common shortcuts therefore vanish instead of producing an ActionPressKey; normalize the literal space to the supported Space key name before applying whitespace validation.
Useful? React with 👍 / 👎.
| granted_minutes = min(additional_minutes, headroom_minutes) | ||
| updated = await database.browser_sessions.update_persistent_browser_session( | ||
| session_id, | ||
| organization_id=organization_id, | ||
| timeout_minutes=current_timeout_minutes + granted_minutes, | ||
| ) |
There was a problem hiding this comment.
When two extension requests overlap, both can read the same current_timeout_minutes, compute the same new value, and then assign it through update_persistent_browser_session, whose separate transaction does not lock or compare the previously read row. Both requests return success but the last write wins—for example, two concurrent 30-minute extensions add only 30 minutes—so perform an atomic capped increment or lock the row across the read-modify-write operation.
Useful? React with 👍 / 👎.
Sync PR: 16454
Sync SHA: 93913c443bc94e1437f3d4ba3aa7e01479aa92d4