Add tests for UI features#695
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds six new UI test suites, shared keyboard test helpers, test runner wiring, and small source changes to exports, gizmo button requirements, and API test process handling. ChangesTest coverage expansion and supporting fixes
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/utils/keyboardDispatcherTestUtils.js (1)
10-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest relies on private
_modeStackinternals.
topHandler()reaches directly intoKeyboardDispatcher._modeStack, an underscore-prefixed (intended-private) field. This works today but couples every keyboard test suite to the dispatcher's internal representation — a refactor of the mode-stack shape inkeyboardDispatcher.jswould silently break these tests without any public-contract signal.Consider exposing a small public test hook (e.g.
KeyboardDispatcher.getActiveHandler()) instead, if that module is easy to touch in this stack.🤖 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 `@tests/utils/keyboardDispatcherTestUtils.js` around lines 10 - 13, The topHandler() test helper is coupled to the private KeyboardDispatcher._modeStack internals, so update it to use a public contract instead. Add or use a small public accessor on KeyboardDispatcher, such as getActiveHandler(), and change topHandler() in the keyboardDispatcherTestUtils helper to call that API rather than reading _modeStack directly.tests/canvas-utils.test.js (1)
148-180: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTests couple to
KeyboardDispatcher._modeStack, a private-looking field.Reaching into an underscore-prefixed internal array ties these tests to implementation details rather than a public API surface. If
KeyboardDispatcherinternals change, these assertions will break even if behavior is otherwise unchanged.Also applies to: 260-268
🤖 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 `@tests/canvas-utils.test.js` around lines 148 - 180, The tests are asserting against KeyboardDispatcher._modeStack, which is an internal implementation detail. Update the affected canvas-utils tests to verify behavior through KeyboardDispatcher’s public API or observable outcomes instead of reading the underscore-prefixed stack directly, and apply the same change in the later assertions referenced by the same pattern.
🤖 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.
Inline comments:
In `@tests/blocklyshadowutil.test.js`:
- Around line 122-129: Update the test comment in
buildColorsListShadowSpec-related coverage so it matches the actual argument
being used. The current note references "cat", but the test calls
buildColorsListShadowSpec('__unknown_object__'); replace the stale historical
mention with a comment that describes the present fallback behavior only,
keeping it aligned with the test's current inputs and intent.
In `@tests/gizmos.test.js`:
- Around line 304-312: The gizmo toggle test only checks rotation and scale
flags while they are still false, so it does not verify that turning off one
gizmo clears other enabled flags. Update the test in gizmos.test.js around
toggleGizmo and mgr.*GizmoEnabled to first enable rotation and scale (via their
corresponding buttons or toggles) before disabling position, then assert all
three flags are cleared after the second toggle. Keep the focus on the shared
state in the Gizmo manager, not just the positionGizmoEnabled flag.
---
Nitpick comments:
In `@tests/canvas-utils.test.js`:
- Around line 148-180: The tests are asserting against
KeyboardDispatcher._modeStack, which is an internal implementation detail.
Update the affected canvas-utils tests to verify behavior through
KeyboardDispatcher’s public API or observable outcomes instead of reading the
underscore-prefixed stack directly, and apply the same change in the later
assertions referenced by the same pattern.
In `@tests/utils/keyboardDispatcherTestUtils.js`:
- Around line 10-13: The topHandler() test helper is coupled to the private
KeyboardDispatcher._modeStack internals, so update it to use a public contract
instead. Add or use a small public accessor on KeyboardDispatcher, such as
getActiveHandler(), and change topHandler() in the keyboardDispatcherTestUtils
helper to call that API rather than reading _modeStack directly.
🪄 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: b1507957-f3f0-4a34-b33e-3c59d2d3180e
📒 Files selected for processing (12)
accessibility/keyboardui.jsscripts/run-api-tests.mjstests/axis-keyboard.test.jstests/blocklyshadowutil.test.jstests/canvas-utils.test.jstests/contextmenu.test.jstests/gizmo-mobile-hud.test.jstests/gizmos.test.jstests/keyboardui.test.jstests/tests.htmltests/utils/keyboardDispatcherTestUtils.jsui/gizmos.js
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/gizmo-mobile-hud.test.js (1)
1-254: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPrettier check is failing in CI.
The pipeline reports a Prettier formatting failure across 200 files, which likely includes this new test file. Run
npx prettier --write .before merging.🤖 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 `@tests/gizmo-mobile-hud.test.js` around lines 1 - 254, Prettier formatting is failing in CI, so this new test file likely needs to be reformatted to match the repo style. Run Prettier on the affected test module that defines runGizmoMobileHudTests and related describe/it blocks, then verify the file is cleanly formatted before merging.Source: Pipeline failures
🧹 Nitpick comments (4)
tests/canvas-utils.test.js (2)
198-204: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest doesn't verify the "+x" movement it claims to test.
The test title says "moves it +x" but the body only asserts the circle gets created — it never checks the circle's x-position actually changed after the ArrowRight keydown. Compare with the sibling "ArrowDown moves +y" test (lines 206-213), which does assert positional delta.
🧪 Suggested fix
it('ArrowRight creates the circle on demand and moves it +x', function () { startCanvasKeyboardMode(() => {}); expect(getCanvasCircle()).to.equal(null); topHandler()(keyEvent({ key: 'ArrowRight' })); - expect(getCanvasCircle()).to.exist; + const circle = getCanvasCircle(); + expect(circle).to.exist; + const canvas = flock.scene.getEngine().getRenderingCanvas(); + const bounds = canvas.getBoundingClientRect(); + expect(parseFloat(circle.style.left) - bounds.left).to.be.greaterThan(bounds.width / 2); dispatchKeyup('ArrowRight'); });🤖 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 `@tests/canvas-utils.test.js` around lines 198 - 204, The ArrowRight test in the canvas keyboard mode suite only verifies circle creation and does not assert the claimed +x movement. Update the test around startCanvasKeyboardMode, topHandler, and getCanvasCircle to capture the circle’s initial x-position, trigger ArrowRight, and then assert the x-value increased (matching the ArrowDown positional-delta pattern) while still confirming the circle is created.
148-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffCoupling tests to
KeyboardDispatcher._modeStackinternal state.Multiple tests reach into the underscore-prefixed
_modeStackinternal field to assert mode push/pop behavior. This makes the tests brittle to internal refactors ofKeyboardDispatcher. Consider exposing a small public introspection helper (e.g.KeyboardDispatcher.getActiveModeNames()) if this pattern is needed elsewhere.Also applies to: 260-270
🤖 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 `@tests/canvas-utils.test.js` around lines 148 - 180, The canvas utils tests are coupled to the internal KeyboardDispatcher._modeStack implementation, which makes them brittle. Update the assertions in the startCanvasKeyboardMode/stopCanvasKeyboardMode tests to use a public introspection API instead of reaching into the underscore-prefixed field, and add a small helper on KeyboardDispatcher such as getActiveModeNames() or similar to verify mode push/pop behavior without exposing internal state. Keep the tests focused on the public behavior of startCanvasKeyboardMode and stopCanvasKeyboardMode.tests/gizmo-mobile-hud.test.js (1)
157-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWeak assertion for repeat-stop behavior.
This test only checks that releasing the arrow doesn't throw; it doesn't verify the repeat timer/interval actually stopped firing
onMove. Consider using fake timers (e.g.,sinon.useFakeTimers) to advance time after release and assertmovesdoesn't grow further, giving real coverage of the repeat-cancellation logic rather than just a non-throw check.🤖 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 `@tests/gizmo-mobile-hud.test.js` around lines 157 - 162, The arrow release test in the gizmo mobile HUD suite only asserts that `btn.onPointerUpObservable.notifyObservers()` does not throw, so it does not verify that the repeat loop actually stops. Update the `releasing the arrow stops the repeat without throwing` test to use fake timers (for example via `sinon.useFakeTimers`), advance time after the pointer-up event, and assert that `onMove` is no longer called and the `moves` count does not increase. Use the existing `make`, `findControl`, and `gizmo-arrow-1` setup to exercise the repeat-cancellation behavior directly.tests/keyboardui.test.js (1)
169-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard
AreaManagercleanup with try/finally to avoid cascading test failures.
AreaManager.toggle(true)is opened at line 170 but only closed at line 174, after the assertion at line 173. If that assertion fails, the overlay stays open — there's noafterEachforAreaManagerin thisdescribe('GizmoMenuManager', ...)block (onlyGizmoMenuManager.toggle(false)runs at 58-59) — so subsequent, unrelated tests can inherit the stuck "OVERLAY" context and fail for the wrong reason.🧪 Proposed fix
it('does nothing while the area menu overlay is open (OVERLAY context)', function () { AreaManager.toggle(true); - const event = { preventDefault() {}, stopPropagation() {} }; - KeyboardDispatcher._registry['*:Mod+KeyG'](event); - expect(GizmoMenuManager.isOpen()).to.equal(false); - AreaManager.toggle(false); + try { + const event = { preventDefault() {}, stopPropagation() {} }; + KeyboardDispatcher._registry['*:Mod+KeyG'](event); + expect(GizmoMenuManager.isOpen()).to.equal(false); + } finally { + AreaManager.toggle(false); + } });🤖 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 `@tests/keyboardui.test.js` around lines 169 - 176, The test in KeyboardDispatcher should always reset AreaManager after opening the overlay, even if the assertion fails. Wrap the AreaManager.toggle(true) / GizmoMenuManager.isOpen() assertion in a try/finally so AreaManager.toggle(false) is guaranteed to run, and keep the cleanup local to the existing “does nothing while the area menu overlay is open” test since this describe block does not have an AreaManager afterEach.
🤖 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.
Inline comments:
In `@tests/contextmenu.test.js`:
- Line 8: The exported test helper runContextMenuTests currently takes an unused
flock parameter, which ESLint flags under no-unused-vars. Keep the shared
test-runner signature intact by renaming the parameter to use an underscore
prefix in runContextMenuTests so it is clearly intentionally unused while
preserving compatibility with the other run*Tests(flock) functions.
---
Outside diff comments:
In `@tests/gizmo-mobile-hud.test.js`:
- Around line 1-254: Prettier formatting is failing in CI, so this new test file
likely needs to be reformatted to match the repo style. Run Prettier on the
affected test module that defines runGizmoMobileHudTests and related describe/it
blocks, then verify the file is cleanly formatted before merging.
---
Nitpick comments:
In `@tests/canvas-utils.test.js`:
- Around line 198-204: The ArrowRight test in the canvas keyboard mode suite
only verifies circle creation and does not assert the claimed +x movement.
Update the test around startCanvasKeyboardMode, topHandler, and getCanvasCircle
to capture the circle’s initial x-position, trigger ArrowRight, and then assert
the x-value increased (matching the ArrowDown positional-delta pattern) while
still confirming the circle is created.
- Around line 148-180: The canvas utils tests are coupled to the internal
KeyboardDispatcher._modeStack implementation, which makes them brittle. Update
the assertions in the startCanvasKeyboardMode/stopCanvasKeyboardMode tests to
use a public introspection API instead of reaching into the underscore-prefixed
field, and add a small helper on KeyboardDispatcher such as getActiveModeNames()
or similar to verify mode push/pop behavior without exposing internal state.
Keep the tests focused on the public behavior of startCanvasKeyboardMode and
stopCanvasKeyboardMode.
In `@tests/gizmo-mobile-hud.test.js`:
- Around line 157-162: The arrow release test in the gizmo mobile HUD suite only
asserts that `btn.onPointerUpObservable.notifyObservers()` does not throw, so it
does not verify that the repeat loop actually stops. Update the `releasing the
arrow stops the repeat without throwing` test to use fake timers (for example
via `sinon.useFakeTimers`), advance time after the pointer-up event, and assert
that `onMove` is no longer called and the `moves` count does not increase. Use
the existing `make`, `findControl`, and `gizmo-arrow-1` setup to exercise the
repeat-cancellation behavior directly.
In `@tests/keyboardui.test.js`:
- Around line 169-176: The test in KeyboardDispatcher should always reset
AreaManager after opening the overlay, even if the assertion fails. Wrap the
AreaManager.toggle(true) / GizmoMenuManager.isOpen() assertion in a try/finally
so AreaManager.toggle(false) is guaranteed to run, and keep the cleanup local to
the existing “does nothing while the area menu overlay is open” test since this
describe block does not have an AreaManager afterEach.
🪄 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: ad74815c-aabf-44a6-a7ed-f89a597f7786
📒 Files selected for processing (12)
accessibility/keyboardui.jsscripts/run-api-tests.mjstests/axis-keyboard.test.jstests/blocklyshadowutil.test.jstests/canvas-utils.test.jstests/contextmenu.test.jstests/gizmo-mobile-hud.test.jstests/gizmos.test.jstests/keyboardui.test.jstests/tests.htmltests/utils/keyboardDispatcherTestUtils.jsui/gizmos.js
1 participant
Summary
AreaManagerto make testing it easier (even though it's not imported anywhere)AI usage
Claude Sonnet 4.6 wrote all tests.
/codereviewran and marked its own work. I verified its findings manually and then asked Claude to either fix them or ignore them.Summary by CodeRabbit