Skip to content

Stabilize gizmo mesh picking and keyboard handlers; fix event listener options#574

Closed
tracygardner wants to merge 1 commit intomainfrom
codex/fix-double-movement-on-keypress
Closed

Stabilize gizmo mesh picking and keyboard handlers; fix event listener options#574
tracygardner wants to merge 1 commit intomainfrom
codex/fix-double-movement-on-keypress

Conversation

@tracygardner
Copy link
Copy Markdown
Contributor

@tracygardner tracygardner commented Apr 26, 2026

Motivation

  • Prevent duplicate mesh picks and accidental multiple callbacks when using pickMeshFromScene or canvas keyboard picking.
  • Avoid reattaching keyboard handlers repeatedly when the same mesh is attached to the position gizmo.
  • Ensure removeEventListener uses the same options as the corresponding addEventListener to reliably unregister the handler.
  • Normalize attached mesh to the root mesh and short-circuit no-op attachments when the same mesh is already attached.

Description

  • In ui/axis-keyboard.js update stop() to call document.removeEventListener("keydown", handler, true) so the removal matches the capturing listener registration.
  • In ui/gizmos.js add hasPicked and a handlePicked wrapper inside pickMeshFromScene to ensure non-persistent picks only trigger once and to centralize cleanup behavior.
  • Refactor position-gizmo keyboard logic by introducing keyboardAttachedMesh and activatePositionKeyboardForMesh() to avoid reattaching the keyboard handler when the same mesh is reselected and to centralize block highlighting.
  • Adjust initial position-gizmo attachment logic to call activatePositionKeyboardForMesh() for the currently attached mesh and to use getRootMesh when picking child meshes.
  • In setGizmoManager normalize incoming meshes by resolving to the root using getRootMesh(mesh.parent) when appropriate and return early if the requested mesh is already attached to avoid redundant work.
  • Ensure scale gizmo Z-axis enabling/disabling logic is preserved when attaching a mesh, and attach a dispose observer for attached meshes as before.
  • Add a keydown listener on the canvas to handle the Delete key (keyCode 46) for mesh deletion (handler body shown partially in diff).

Testing

  • Ran the project's automated unit test suite with npm test, and the tests completed successfully.
  • Ran linting with npm run lint and performed a production build with npm run build, and both succeeded.
  • Exercised the gizmo pick/attach and keyboard flows in automated UI checks, and no regressions were detected.

Codex Task

Summary by CodeRabbit

Bug Fixes

  • Keyboard event listeners now deregister properly with correct capture configuration on shutdown
  • Gizmo picking behavior improved to prevent duplicate callback executions per interaction cycle
  • Keyboard mesh attachment and reattachment logic refined for more stable gizmo interactions

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 26, 2026

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

Two files updated: axis-keyboard.js fixes event listener deregistration by including the correct capture flag to match registration; gizmos.js refactors pick handling deduplication, keyboard-move activation tracking, and mesh attachment normalization to eliminate redundant operations.

Changes

Cohort / File(s) Summary
Event Listener Deregistration Fix
ui/axis-keyboard.js
The stop() method now removes the keydown event listener with capture=true to match the capture phase configuration used during registration, ensuring proper listener cleanup.
Gizmo Pick and Attachment Refactoring
ui/gizmos.js
Consolidated pick handling via hasPicked flag and shared handlePicked helper to prevent duplicate cleanup/callbacks; refactored keyboard-move activation to track currently attached mesh and reattach only on changes; normalized attached mesh to root mesh upfront and simplified scale gizmo Z-axis disable logic in setGizmoManager.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Poem

🐰 A listener cleaned with proper care,
No capture flag left hanging there,
Picks now deduplicated bright,
Meshes normalized just right—
Gizmos flow through cleaner skies! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: stabilizing gizmo mesh picking, fixing keyboard handlers, and correcting event listener options (capture flag).
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-double-movement-on-keypress

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cloudflare-workers-and-pages
Copy link
Copy Markdown

Deploying flockxr with  Cloudflare Pages  Cloudflare Pages

Latest commit: 90232a7
Status: ✅  Deploy successful!
Preview URL: https://d35d9b5c.flockxr.pages.dev
Branch Preview URL: https://codex-fix-double-movement-on.flockxr.pages.dev

View logs

@tracygardner tracygardner deleted the codex/fix-double-movement-on-keypress branch April 26, 2026 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant