Axis keyboard handler and Move gizmo keyboard controls#546
Axis keyboard handler and Move gizmo keyboard controls#546tracygardner merged 2 commits intoflipcomputing:mainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 39 minutes and 29 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR introduces a new keyboard controller for axis-constrained movement and integrates it into the gizmo system. The Changes
Sequence DiagramsequenceDiagram
participant User
participant Keyboard Handler
participant Gizmo System
participant Mesh
participant Block Storage
User->>Gizmo System: Start position gizmo on mesh
Gizmo System->>Keyboard Handler: Create with onMove/onConfirm/onCancel
Keyboard Handler->>Keyboard Handler: Begin listening for key events
loop Movement Phase
User->>Keyboard Handler: Press arrow key (or x/y/z/PageUp/PageDown)
Keyboard Handler->>Keyboard Handler: Calculate delta based on constraint & key
Keyboard Handler->>Mesh: Apply incremental position delta
Mesh->>User: Visual feedback (mesh moves)
end
alt User Confirms
User->>Keyboard Handler: Press Enter or Space
Keyboard Handler->>Block Storage: Persist final mesh position
Block Storage-->>Keyboard Handler: Position saved
Keyboard Handler->>Gizmo System: onConfirm callback
else User Cancels
User->>Keyboard Handler: Press Escape
Keyboard Handler->>Mesh: Restore original position
Keyboard Handler->>Gizmo System: onCancel + disable gizmos
end
Keyboard Handler->>Keyboard Handler: Stop listening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/axis-keyboard.js`:
- Around line 14-17: The keyboard handler function (handler) is intercepting
movement keys even when the user is typing; update handler to early-return when
the event target is an editable element (input, textarea, select) or when
target.closest('[contenteditable="true"]') is true or document.activeElement is
an input-like element, and only process keys when not editing; also avoid
calling preventDefault()/stopPropagation for those cases. Apply the same check
to the other keyboard-handling block(s) (the code covering the
arrow/x/y/z/Enter/Space/Escape logic) so movement controls are ignored while
focus is in editable controls.
- Around line 83-95: The key handler currently calls onConfirm()/onCancel() then
stop(), but if those callbacks throw the listener isn't removed; update the
keyboard handler (the switch cases handling "Enter"/" " and "Escape" in
ui/axis-keyboard.js) to ensure stop() always runs by invoking onConfirm() and
onCancel() inside a try/finally (or equivalent) so that stop() is executed in
the finally block even if the callback throws; keep event.preventDefault()
before calling callbacks and preserve calling stop() and break behavior.
In `@ui/gizmos.js`:
- Around line 845-868: The delayed setTimeout can register a keyboard handler
for a mesh that is no longer the active gizmo; to fix, capture an identifying
token when scheduling (e.g., const scheduledBlockKey = mesh?.metadata?.blockKey
or scheduledMeshId = mesh.id) and inside the setTimeout callback first verify
the gizmo state still matches (e.g., currentActiveMesh or currentBlockKey ===
scheduledBlockKey and gizmos are still enabled) before assigning
stopAxisKeyboard = createAxisKeyboardHandler(...); if the check fails, return
without registering the handler and/or clear any state, ensuring you reference
stopAxisKeyboard, createAxisKeyboardHandler, mesh (mesh?.metadata?.blockKey or
mesh.id), meshMap, and disableGizmos in your guard logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c472032-0b1e-4d9e-b0cb-661aa27f7583
📒 Files selected for processing (2)
ui/axis-keyboard.jsui/gizmos.js
| function handler(event) { | ||
| const step = event.shiftKey ? stepFast : stepNormal; | ||
|
|
||
| switch (event.key) { |
There was a problem hiding this comment.
Avoid intercepting movement keys while the user is typing.
At Line 14, the handler processes x/y/z, arrows, Enter/Space/Escape globally without checking editable targets. This can move meshes while editing form fields and suppress expected text/navigation keys.
💡 Suggested fix
export function createAxisKeyboardHandler({
onMove,
onConfirm,
onCancel,
stepNormal = 0.1,
stepFast = 1,
}) {
let axis = null;
+ function isTypingTarget(target) {
+ const tag = (target?.tagName || "").toLowerCase();
+ return (
+ target?.isContentEditable ||
+ tag === "input" ||
+ tag === "textarea" ||
+ tag === "select"
+ );
+ }
+
function handler(event) {
+ if (isTypingTarget(event.target)) return;
const step = event.shiftKey ? stepFast : stepNormal;Also applies to: 51-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/axis-keyboard.js` around lines 14 - 17, The keyboard handler function
(handler) is intercepting movement keys even when the user is typing; update
handler to early-return when the event target is an editable element (input,
textarea, select) or when target.closest('[contenteditable="true"]') is true or
document.activeElement is an input-like element, and only process keys when not
editing; also avoid calling preventDefault()/stopPropagation for those cases.
Apply the same check to the other keyboard-handling block(s) (the code covering
the arrow/x/y/z/Enter/Space/Escape logic) so movement controls are ignored while
focus is in editable controls.
There was a problem hiding this comment.
I'm not sure that this is a problem. This handler only is active if you're using a specific gizmo, and I don't think that someone would expect to be able to type in a box whilst in the middle of using a gizmo? I might be missing something though.
There was a problem hiding this comment.
@lawsie, that's a fair point. Since the handler is only registered while the move gizmo is actively in use, the scenario where a user would simultaneously have focus in a text input is very unlikely in practice — 3D editor tools generally own keyboard focus for the duration of a gizmo operation. I'll defer to your knowledge of flock's UI here.
It would only become a concern if the UI were ever extended to show editable form fields (e.g., a coordinate input panel) while the gizmo is active at the same time, but that's a future consideration rather than a current issue.
✏️ Learnings added
Learnt from: lawsie
Repo: flipcomputing/flock PR: 546
File: ui/axis-keyboard.js:14-17
Timestamp: 2026-04-16T16:09:17.715Z
Learning: In ui/axis-keyboard.js (JavaScript, BabylonJS), the `createAxisKeyboardHandler` keyboard handler is intentionally registered only while the move gizmo is active (`handlePositionGizmo()` in ui/gizmos.js). A guard against editable input targets (input, textarea, etc.) is NOT required because the handler is scoped to active gizmo use and the UX pattern does not allow typing in form fields simultaneously with gizmo operation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: lawsie
Repo: flipcomputing/flock PR: 544
File: ui/gizmos.js:381-390
Timestamp: 2026-04-16T13:21:14.053Z
Learning: In ui/gizmos.js (JavaScript, BabylonJS), `disableGizmos()` is called internally by BabylonJS during normal gizmo/mesh operations. Therefore, duplicate-mode teardown (removing activeDuplicatePickHandler, clearing button highlights) must NOT be placed inside `disableGizmos()` — it would remove the click listener before a mouse user can place their duplicate. Cleanup is intentionally placed only in explicit user-gesture handlers: `toggleGizmo()` and the Escape keydown handler.
There was a problem hiding this comment.
@tracygardner I'm going to leave this unless you can see an issue, for the reasons above!
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
2 participants
Summary
Usage
With a mesh selected, select the move gizmo, then:
If no axis fixed:
ArrowLeft/Right moves on X
ArrowUp/Down moves on Z
PageUp/Down moves on Y (mirrors flycam controls)
Holding Shift moves by larger increments
Enter / Space - confirms movement
Escape - restores original mesh position
A message is displayed in the canvas: either X axis, Y axis, Z axis or free, depending on which movement mode you are in.
AI involvement
I drafted a plan for this implementation after research on Blender functionality/talking to Tracy. Claude Sonnet 4.6 provided the steps to implement my plan. All steps were implemented manually and code was checked so that I understood its function.
Summary by CodeRabbit