Skip to content

Refactor/keyboard controller#7761

Open
vanshika2720 wants to merge 2 commits into
sugarlabs:masterfrom
vanshika2720:refactor/keyboard-controller
Open

Refactor/keyboard controller#7761
vanshika2720 wants to merge 2 commits into
sugarlabs:masterfrom
vanshika2720:refactor/keyboard-controller

Conversation

@vanshika2720

@vanshika2720 vanshika2720 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR optimizes node add/remove operations in the scheduler cache by eliminating O(n) NodeList scans performed while holding the global scheduler cache mutex.

Previously, AddOrUpdateNode() performed a linear scan over NodeList to determine whether a node already existed before appending it. Similarly, RemoveNode() scanned the slice to locate the node before removing it, resulting in O(n) lookup and additional O(n) slice copying.

To improve scalability, this PR introduces a nodeListIndex (map[string]int) that is maintained alongside NodeList. The index provides O(1) membership checks and enables efficient node removal using swap-delete while keeping the index synchronized.

AddOrUpdateNode() now uses the index map to determine whether a node already exists before appending it. RemoveNode() performs O(1) lookup through the index map, replaces the removed entry with the last element in the slice, updates the swapped node's index, and truncates the slice.

To preserve compatibility with existing tests that construct SchedulerCache directly, the index is lazily initialized from an existing NodeList when necessary.

No behavioral changes are intended. The scheduler cache continues to expose the same functionality, and NodeList ordering remains effectively unchanged from the scheduler's perspective since ordering is not relied upon by snapshot generation.

Test Plan

Added and updated unit tests covering:

  • Duplicate node additions do not create multiple NodeList entries.
  • Node removal correctly updates both NodeList and nodeListIndex.
  • Removing and re-adding the same node behaves correctly.
  • nodeListIndex remains consistent after every add and remove operation.
  • Existing scheduler cache tests continue to pass.

Added benchmarks for node add/remove operations at 100-, 500-, and 2000-node scale to compare the optimized implementation against the previous linear-scan approach.

Additional Notes

This optimization is internal to the scheduler cache and does not introduce any user-facing changes or API modifications.

The implementation reduces mutex hold time during node add/remove events, improving scalability and reducing lock contention in large clusters while preserving existing scheduler behavior.

Only files directly related to this optimization were modified:

  • pkg/scheduler/cache/event_handlers.go
  • pkg/scheduler/cache/cache.go (index initialization)
  • pkg/scheduler/cache/*_test.go
  • pkg/scheduler/cache/*_benchmark_test.go
  • Documentation

Move keyboard shortcut dispatch, modifier key handling (Alt/Ctrl/Shift),
arrow key navigation, space bar play/stop, copy/paste shortcuts, tempo
widget integration, and current-key-code state from activity.js into a
new js/keyboard-controller.js module. The module exposes a clean public
API via setupKeyboardController(activity) and delegates all calls from
activity to the new KeyboardController class. No behavioral changes;
all existing keyboard shortcuts are preserved exactly.

What was moved
- __keyPressed - full shortcut dispatch (~390 lines)
- getCurrentKeyCode / clearCurrentKeyCode - key-code state used by the
  Keyboard Sensor block
- Keyboard listener registration and disposal (own document-level
  addEventListener/removeEventListener, independent of Activity's
  request-scoped listener tracking so the shortcut keeps working for
  the lifetime of the page, matching prior behavior)

loader.js: add "keyboard-controller" to the activity/activity shim
deps list and paths map, matching the existing controller extraction
pattern.

Updated activity_toolbar_integration.test.js: the "keyboard-triggered
execution shortcuts" tests exercised __keyPressed directly and now
mock setupKeyboardController like the other controller setup calls;
equivalent (and expanded) coverage moves to the new dedicated
keyboard-controller.test.js suite.

Signed-off-by: Vanshika <pahalvanshikaa@gmail.com>
Add a dedicated Jest suite for KeyboardController covering setup,
current-key-code state, listener lifecycle (registered on setup,
removed by dispose, dispose is idempotent), arrow key navigation,
space bar play/stop, copy/paste shortcuts, tempo widget integration,
Alt/Ctrl/Shift modifier combinations, ignoring shortcuts while a text
input is focused, palette scrolling, and stage scrolling.

Tests assert on observable effects (blocks moving, playback starting/
stopping, tempo changing, stage scrolling) rather than on internal
calls like addEventListener/removeEventListener, so the suite survives
future implementation changes.

Signed-off-by: Vanshika <pahalvanshikaa@gmail.com>
@github-actions github-actions Bot added documentation Updates to docs, comments, or README size/XXL XXL: 1000+ lines changed area/javascript Changes to JS source files area/tests Changes to test files labels Jul 8, 2026
@vanshika2720 vanshika2720 marked this pull request as ready for review July 8, 2026 14:44
@vanshika2720

Copy link
Copy Markdown
Contributor Author

The failing Jest CI is unrelated to this refactor. I verified the same failures reproduce on a clean upstream/master and originate from the recently merged test PRs #7646 and #7647.

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

This PR has merge conflicts with master.

Please rebase your branch:

# Add upstream remote (one-time setup)
git remote add upstream https://github.com/sugarlabs/musicblocks.git

# Fetch latest master and rebase
git fetch upstream
git rebase upstream/master

# Resolve any conflicts, then:
git push --force-with-lease origin YOUR_BRANCH

Tip: Enable "Allow edits from maintainers" on this PR so we can auto-rebase for you next time. This only grants access to your PR branch. Your fork's other branches are not affected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/javascript Changes to JS source files area/tests Changes to test files documentation Updates to docs, comments, or README needs-rebase size/XXL XXL: 1000+ lines changed

Projects

Development

Successfully merging this pull request may close these issues.

1 participant