Fix issue #350: deterministic notation stacking order#351
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes unstable notation stacking (issue #350) by making ListCheckbox emit selected values in a deterministic, canonical order so keyboard notation layers render consistently.
Changes:
- Updated
ListCheckboxto emit selected values in a stable order derived from theoptionslist (while retaining non-menu selections present ininitOptions). - Added prop-sync behavior so local checkbox state rebuilds when
options/initOptionschange. - Added regression tests covering toggle-order stability and
initOptionsprop updates.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/components/form/ListCheckbox.js | Implements canonical-order emission and prop-sync state rebuilding. |
| src/test/ListCheckbox.order.test.js | Adds regression tests for deterministic emission and prop syncing. |
| .github/copilot/tasks/issue-350-order-of-notations/task.md | Documents investigation, plan, and validation for issue #350. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- .github/copilot/: project-context, constraints, dependencies, copilot-instructions - .github/skills/flows/: 6 flow docs + hub (app-bootstrap, scale-change-pipeline, note-playback, session-share, video-overlay, settings-propagation) - .github/hooks/: constraint-guard, dangerous-command-guard, flow-verification-reminder, constraint-forwarding - package.json: add "lint" script (eslint src --ext .js,.jsx) and eslint devDependency aligned with existing lockfile chain - tasks/dal-a-project-setup/: completed setup task record
Rewrite task.md to reflect decision to revert the prior fix commit fb80ceb and replan using state-boundary normalization in WholeApp. Documents slice-by-slice roadmap and confirmed visual stacking direction.
This reverts commit fb80ceb. Reason: the prior fix edited the generic ListCheckbox component rather than the real ordering owner (WholeApp state + Keyboard/ColorKey render pipeline) and missed the session-restore path. Replanning per task file with Option 1 (state-boundary normalization in WholeApp). Conflict resolution: kept the current HEAD version of the issue-350 task.md (it was rewritten on this branch with the new roadmap after fb80ceb was authored).
…order Two focused integration tests lock the visual contract that notation layers render in the canonical Notation-menu order (Chord extensions on top, English at bottom) regardless of the order in which the notation array is written into WholeApp state. Test 1 (menu writer path): calls instance.handleChangeNotation with a scrambled array and asserts canonical DOM order. Test 2 (session-restore path): simulates openSavedSession by directly setting state.notation to a legacy out-of-order array and asserts canonical DOM order. Both fail today because WholeApp writes the notation array verbatim into state and Keyboard/ColorKey render in that literal order. Slice 3 will add normalizeNotationOrder at every writer seam to make these pass.
…writer
Add src/Model/normalizeNotationOrder.js exporting a pure, idempotent
helper that re-orders any notation array to the canonical Notation-menu
order ("Colors" first, then Chord extensions, Scale Steps, Relative,
Romance, German, English). Unknown values are preserved at the end for
forward-compat and malformed session payloads.
Wire the helper into WholeApp with defense-in-depth (Approach 3):
- handleChangeNotation now normalizes the selected notation array
emitted by the Notation menu's ListCheckbox callback.
- openSavedSession now normalizes result.notation before setState, so
legacy or out-of-order session documents cannot carry a scrambled
array into live state.
- componentDidUpdate acts as a safety net: any writer that reaches
state.notation (direct setState, future writers) is re-normalized,
making canonical order a true invariant of WholeApp state rather than
a convention.
Verification: both tests in src/__integration__/notation-order.test.js
now pass, and adjacent WholeApp integration suites (scale-visualization,
menu-staff-integration, state-flow) still pass (34/34).
…opagation Fix #350 made canonical Notation-menu order a true invariant of WholeApp state.notation. Record the enforcement points (handleChangeNotation, openSavedSession, componentDidUpdate) and the normalizeNotationOrder helper in the settings-propagation flow doc so future work knows this invariant exists and where it is enforced.
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #350 by enforcing a deterministic, canonical stacking order for notation label rows rendered on keys, ensuring the UI matches the Notation menu’s intended ordering even after menu interactions and session restores. It also adds DAL-A-related repo documentation/hook scaffolding and a standalone lint script.
Changes:
- Introduce
normalizeNotationOrderand apply it atWholeAppnotation writer boundaries (menu + session restore) plus acomponentDidUpdatesafety net to enforce a state invariant. - Add integration coverage to lock the DOM/visual stacking order regardless of input array ordering.
- Add DAL-A scaffolding: flow docs, Copilot project docs/constraints, hook scripts, and a
lintnpm script/devDependency.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/integration/notation-order.test.js | Adds integration tests asserting canonical DOM order for notation rows via menu writer and direct state writes. |
| src/WholeApp.js | Normalizes notation on write (menu + session restore) and enforces canonical order via componentDidUpdate. |
| src/Model/normalizeNotationOrder.js | New pure helper to reorder notation arrays into canonical order while preserving unknown values. |
| package.json | Adds standalone lint script and adds eslint devDependency. |
| .github/skills/flows/video-overlay.md | Adds runtime flow doc for the video overlay path. |
| .github/skills/flows/settings-propagation.md | Adds flow doc and documents the new state.notation canonical-order invariant. |
| .github/skills/flows/session-share.md | Adds runtime flow doc for session sharing/restoration. |
| .github/skills/flows/scale-change-pipeline.md | Adds runtime flow doc for scale selection → keyboard rebuild. |
| .github/skills/flows/note-playback.md | Adds runtime flow doc for keyboard input → audio playback. |
| .github/skills/flows/app-bootstrap.md | Adds runtime flow doc for app startup and session hydration. |
| .github/skills/flows/SKILL.md | Adds a catalog/index for the flow docs. |
| .github/hooks/scripts/flow-verification-reminder.py | Adds hook script to remind updating flow docs when edited files are referenced by a flow. |
| .github/hooks/scripts/dangerous-command-guard.py | Adds hook script to flag potentially destructive terminal commands. |
| .github/hooks/scripts/constraint-guard.py | Adds hook script enforcing DAL-A constraints on edits to sensitive paths. |
| .github/hooks/scripts/constraint-forwarding.py | Adds hook script to forward constraint summaries to subagents. |
| .github/hooks/hooks.json | Wires up the hook scripts for PreToolUse/PostToolUse/SubagentStart. |
| .github/copilot/tasks/issue-350-order-of-notations/task.md | Adds task tracking doc for the issue #350 work and decisions. |
| .github/copilot/tasks/dal-a-project-setup/task.md | Adds task tracking doc for DAL-A setup work. |
| .github/copilot/project-context.md | Adds Copilot-oriented project context (stack, structure, conventions). |
| .github/copilot/dependencies.md | Adds Copilot-oriented dependency inventory. |
| .github/copilot/copilot-instructions.md | Adds repo-local Copilot usage instructions and references. |
| .github/copilot/constraints.md | Adds DAL-A constraints (protected paths, security boundaries, fragile areas, etc.). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- normalizeNotationOrder.js: rephrase idempotency contract as content equality (function returns a new array instance, so === is not guaranteed). Correct the "Colors" comment: the case "Colors" branch in Keyboard is a no-op; entry is preserved for backward compatibility. - task.md: consolidate the two conflicting visual-direction rows in the Decisions table into a single canonical row (DOM order = canonical list, Chord extensions on top, English at bottom). - dependencies.md: align eslint version with package.json (^8.3.0). - dangerous-command-guard.py: fix `rm -rf .` regex so it matches `.` followed by /, whitespace, or end-of-string (previously required a trailing whitespace and missed the common end-of-line form).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #350 — the notation layers on each key no longer appear in random/reversed order after menu interactions or session loads. The rendered stack now always matches the canonical Notation-menu order, top-to-bottom: Chord extensions → Scale Steps → Relative → Romance → German → English (Chord extensions on top, English at bottom).
Approach (state-boundary normalization)
The earlier attempt (now reverted) patched the generic
ListCheckboxcomponent, but discovery showed the real ownership wasWholeApp.state.notationplus the session-restore path that bypassed any menu-layer normalization. This PR takes a state-layer approach instead:src/Model/normalizeNotationOrder.js— idempotent, never mutates, preserves unknown values defensively.WholeApp.handleChangeNotationnormalizes the selected notation array beforesetState.WholeApp.openSavedSessionnormalizesresult.notationbeforesetState, so legacy/out-of-order session documents can no longer carry a scrambled array into live state.WholeApp.componentDidUpdatesafety net re-normalizesstate.notationafter any other writer (directsetState, future writers), making canonical order a true invariant of component state rather than a convention.This is documented as a new invariant in
.github/skills/flows/settings-propagation.md.Tests
New integration tests at src/integration/notation-order.test.js cover both writer paths:
handleChangeNotationwith a scrambled array and asserts the rendered.noteWrapper--*DOM order is canonical.state.notationdirectly to a legacy out-of-order array (simulatingopenSavedSession) and asserts canonical DOM order.Both tests fail on the baseline and pass with the fix.
Test status:
MusicScalerelative-syllable logic,CustomVideoPlayerintegration) were verified to fail identically onorigin/master— pre-existing, unrelated to this fix.Commit structure
chore(dal-a)— DAL-A project intelligence and lint script (unrelated scaffolding included in the same task)docs(task)— task-tracking file rewriteRevert "Fix deterministic notation stacking order for issue #350"— undo of the prior approach (fb80ceb)test(issue-350)— failing integration tests (red baseline)fix(issue-350)— helper + writer normalization + lifecycle safety netdocs(flows)— record the new invariant in the flow docRisk
MusicScale.js), render components (Keyboard.js,ColorKey.js), or theListCheckboxpublic API.componentDidUpdatesafety net triggers a second render only when an un-normalized write occurs; common menu/restore paths eagerly normalize and render correctly on the first frame.Closes #350