diff --git a/.github/copilot/constraints.md b/.github/copilot/constraints.md new file mode 100644 index 00000000..55c0aef1 --- /dev/null +++ b/.github/copilot/constraints.md @@ -0,0 +1,49 @@ +# Constraints + +## Hard Rules + +- Preserve the integration-first testing strategy documented in `docs/TESTING.md` and avoid replacing behavior coverage with narrow unit tests. +- Keep relative notation key-independent across all 12 chromatic roots and do not regress other notation modes when modifying scale generation. +- Maintain keyboard accessibility for interactive controls, including Enter/Space activation, focus visibility, and focus return from overlays. +- Treat Yarn as the package manager for repo workflows; if a shell lacks `yarn`, use `corepack yarn` rather than switching the repo guidance to npm. +- When runtime behavior changes in a documented path, update the matching flow doc in `.github/skills/flows/`. + +## Protected Contracts + +| Contract | Pattern | Enforcement | Why | +| --- | --- | --- | --- | +| Music scale generation and notation contracts | `src/Model/MusicScale.js` | `ask` | This file defines scale construction and all notation outputs, including relative solfege invariants used across the app. | +| App state orchestration and session hydration | `src/WholeApp.js` | `ask` | `WholeApp` owns most state transitions, menu callbacks, and Firebase session serialization/deserialization. | +| Shared-session route handoff | `src/WholeAppWrapper.js` | `ask` | The wrapper is the only bridge from React Router params into the class-based app shell. | + +## Security Boundaries + +| Boundary | Pattern | Enforcement | Why | +| --- | --- | --- | --- | +| Firebase client persistence boundary | `src/Firebase.js` | `ask` | This file controls client access to Firestore and is the only persistence boundary in the repo. | +| Session sharing surface | `src/components/menu/Share*.js` | `ask` | Share-link creation crosses from local UI state into persisted remote session data. | +| User-supplied video URL handling | `src/components/menu/CustomVideoPlayer.js` | `ask` | Video URLs flow into ReactPlayer or iframe embeds and should be treated as security-sensitive input handling. | + +## Fragile Areas + +| Area | Pattern | Enforcement | Why | +| --- | --- | --- | --- | +| Keyboard interaction and module-scoped state | `src/components/keyboard/**` | `ask` | Keyboard input, audio triggering, and module-level mutable state are tightly coupled and easy to regress. | +| Top-level menu fan-out | `src/components/menu/TopMenu.js` | `ask` | This file fans user actions into most app handlers and affects multiple runtime flows at once. | +| Accessibility integration tests | `src/__integration__/accessibility/**` | `ask` | These tests are the strongest automated guard for keyboard and aria regressions. | +| Relative-notation regression tests | `src/__test__/**/*relative*.test.js` | `ask` | These tests protect a domain invariant that has regressed before and should not be casually edited. | + +## Performance Boundaries + +| Path | Constraint | +| --- | --- | +| `src/Model/MusicScale.js` | Keep scale and notation generation effectively constant-time for the current recipe lookup strategy. | +| `src/components/keyboard/**` | Avoid unnecessary full-keyboard rebuilds or extra global DOM queries on every interaction. | +| `e2e/**` | Keep individual E2E cases within the 30-second Playwright timeout documented by the repo config. | + +## Dangerous Commands + +| Pattern | Why | +| --- | --- | +| `rm\s+-rf\s+build` | Build output removal is acceptable only when the user explicitly asks for a clean rebuild. | +| `git\s+push\s+.*--force` | Force-pushing on this repo can overwrite collaborator history. | diff --git a/.github/copilot/copilot-instructions.md b/.github/copilot/copilot-instructions.md new file mode 100644 index 00000000..2b4b3995 --- /dev/null +++ b/.github/copilot/copilot-instructions.md @@ -0,0 +1,24 @@ +This project uses the copilot development system. + +Active tasks: check `.github/copilot/tasks/`. If `task.md` phase is not `done`, read it first because the Changelog section is the recovery log. +Project context: `project-context.md` for stack, domain, module layout, and conventions. +Constraints: `constraints.md` before modifying code. Sections with `Pattern` and `Enforcement` columns are hook-enforced and may require user confirmation. +Hooks: `.github/hooks/` contains project guardrails. Do not change hook scripts without explicit user approval. +Flows: `.github/skills/flows/` contains the runtime flow catalog. Read the affected flow doc before behavior changes and update it after behavior changes. +Project skills: repo-local skills live under `.github/skills/` and should be preferred when relevant. + +## Project-Specific Guidance + +- This repo is a React 18 music-education SPA with a central `WholeApp` state container, `MusicScale` domain logic, `SoundMaker` audio adapters, and Firebase-backed session sharing. +- The repo uses Yarn workflows. If a shell lacks a global `yarn`, use `corepack yarn` instead of rewriting commands to npm. +- Testing is integration-first and accessibility-heavy. Preserve `docs/TESTING.md` expectations, especially the a11y layers and 100% coverage target. +- Changes in `src/Model/MusicScale.js`, `src/components/keyboard/`, `src/components/menu/CustomVideoPlayer.js`, or session-sharing files should trigger a flow review because they affect documented runtime paths. +- Treat `CLAUDE.md` as a source of domain-specific constraints, especially relative-notation invariants and accessibility expectations. + +## Documentation References + +- See `README.md` for install, dev server, build, deploy, and top-level testing commands. +- See `docs/TESTING.md` for the testing strategy, accessibility test layers, coverage expectations, and the test distribution validator. +- See `CLAUDE.md` for repo-specific accessibility patterns, relative-notation rules, and feature-derived development constraints. + +Use `/begin-task` to start work. Use `/flows` to trace runtime behavior. Use `/flow-impact` to compare before and after behavior. \ No newline at end of file diff --git a/.github/copilot/dependencies.md b/.github/copilot/dependencies.md new file mode 100644 index 00000000..3389e59f --- /dev/null +++ b/.github/copilot/dependencies.md @@ -0,0 +1,36 @@ +# Dependencies + +## Core + +| Dependency | Version | Purpose | Notes | +| --- | --- | --- | --- | +| react | ^18.2.0 | Primary UI framework | App is still organized around a large class-based root component. | +| react-dom | ^18.2.0 | DOM renderer | Used with `createRoot` in the SPA entry point. | +| react-router-dom | ^6.3.0 | Client-side routing | Handles `/` and `/shared/:sessionId` flows. | +| react-scripts | 5.0.1 | Build and test toolchain | CRA stack with embedded ESLint config and Jest integration. | +| firebase | ^10.9.0 | Firestore session persistence | Client SDK only; session sharing depends on it. | +| tone | ^14.7.77 | Audio synthesis support | Used via `SoundMaker` adapters for playback. | +| @tonejs/piano | ^0.2.1 | Sampled piano playback | Tone.js-backed piano instrument. | +| soundfont-player | ^0.12.0 | Alternate audio backend | Present behind the adapter layer. | +| vexflow | ^4.0.3 | Music notation rendering | Used for staff rendering surfaces. | +| react-bootstrap | ^2.5.0 | Menu and UI components | Used in the top menu and related controls. | +| react-player | ^2.10.1 | Embedded video playback | Powers the custom video overlay. | +| react-draggable | ^4.4.5 | Draggable overlay behavior | Used by the custom video player UI. | +| react-tooltip | ^4.2.21 | Tooltip UI | Drives help and hover guidance across controls. | +| sass | ^1.54.9 | SCSS compilation | Styles are organized under `src/styles/`. | +| webmidi | ^2.0.0 | MIDI device input | Supports external keyboard interaction. | + +## Dev + +| Dependency | Version | Purpose | +| --- | --- | --- | +| eslint | ^8.3.0 | Standalone lint command for repo validation and DAL-A setup workflows | +| jest | ^29.0.3 | Test runner for unit and integration coverage | +| @testing-library/react | ^13.0.0 | Integration testing of React components | +| @testing-library/jest-dom | ^5.16.5 | DOM-focused Jest matchers | +| @testing-library/user-event | ^13.2.1 | User interaction simulation in tests | +| jest-axe | ^10.0.0 | Accessibility assertions in Jest | +| @playwright/test | ^1.56.1 | End-to-end testing | +| @axe-core/playwright | ^4.11.0 | Accessibility assertions in Playwright | +| @axe-core/react | ^4.11.0 | Development-time accessibility auditing | +| react-test-renderer | ^18.2.0 | Snapshot and renderer-based tests | diff --git a/.github/copilot/project-context.md b/.github/copilot/project-context.md new file mode 100644 index 00000000..91813856 --- /dev/null +++ b/.github/copilot/project-context.md @@ -0,0 +1,88 @@ +--- +project: "notio" +domain: "frontend" +stack: + language: "JavaScript ES6+" + framework: "React 18 with React Router v6" + build: "react-scripts (CRA)" + package_manager: "yarn (via Corepack when yarn is not globally installed)" + test_framework: "Jest, React Testing Library, Playwright" +architecture: "single-page React application with a central WholeApp state container, model helpers for music/audio logic, and Firebase-backed session sharing" +--- + +# Project Context + +## Module Structure + +```text +src/ + components/ + Model/ + data/ + styles/ + __integration__/ + __test__/ +e2e/ + accessibility/ +docs/ +.github/ + copilot/ + hooks/ + skills/ +``` + +## Patterns + +| Concern | Approach | +| --- | --- | +| Error handling | UI state is managed centrally in `WholeApp`; some async flows still degrade silently and should be treated carefully during edits | +| State management | `WholeApp` owns app state and passes handlers/props down into menus, keyboard, overlays, and wrappers | +| Data access | Firebase Firestore is used directly from the client for shareable session persistence | +| Testing | Integration-first testing with accessibility coverage and Playwright for end-to-end validation | + +## Conventions + +- Preserve the integration-first testing split documented in the repo: integration tests first, E2E second, unit tests only for edge cases. +- Treat `src/Model/MusicScale.js` as a protected domain contract: notation arrays and relative solfege behavior must stay backward-compatible. +- Keep accessibility semantics on interactive UI elements, especially keyboard handlers, `aria-label`, and focus return behavior. +- Use Yarn-oriented workflow and commands for this repo; when the shell lacks a global `yarn`, use `corepack yarn`. +- Update the relevant flow doc under `.github/skills/flows/` whenever behavior changes in a documented runtime path. + +## Existing Documentation + +| Document | Covers | +| --- | --- | +| `README.md` | Project purpose, install/run/build/deploy commands, testing entry points, and dependency notes | +| `docs/TESTING.md` | Integration-first testing policy, accessibility testing layers, coverage requirements, and test distribution validation | +| `CLAUDE.md` | Repo-specific development guidance, accessibility patterns, relative-notation invariants, and recent feature constraints | + +## System Flow Overview + +```mermaid +flowchart TB + subgraph routing["Routing and App Shell"] + A["index.js routes"] --> B["WholeApp or WholeAppWrapper"] + end + subgraph state["App State and Domain"] + B --> C["WholeApp state handlers"] + C --> D["MusicScale and SoundMaker"] + end + subgraph ui["Interactive Surfaces"] + D --> E["Keyboard rendering and playback"] + C --> F["TopMenu, overlays, sharing"] + end + subgraph persistence["Persistence"] + C --> G["Firebase session save and restore"] + end +``` + +## Critical Flows + +| Flow | Link | +| --- | --- | +| app-bootstrap | [→](../skills/flows/app-bootstrap.md) | +| scale-change-pipeline | [→](../skills/flows/scale-change-pipeline.md) | +| note-playback | [→](../skills/flows/note-playback.md) | +| session-share | [→](../skills/flows/session-share.md) | +| video-overlay | [→](../skills/flows/video-overlay.md) | +| settings-propagation | [→](../skills/flows/settings-propagation.md) | diff --git a/.github/copilot/tasks/dal-a-project-setup/task.md b/.github/copilot/tasks/dal-a-project-setup/task.md new file mode 100644 index 00000000..6c6c22f4 --- /dev/null +++ b/.github/copilot/tasks/dal-a-project-setup/task.md @@ -0,0 +1,26 @@ +--- +title: "DAL-A project setup" +slug: "dal-a-project-setup" +phase: "done" +goal: "Initialize DAL-A project intelligence for this repo by scanning conventions and docs, documenting flows, and generating .github/copilot, flow docs, and applicable hooks." +current_action: "Generated DAL-A project files, repo-local flow docs, and hook scaffolding; validation completed with an environment note on lint installation." +--- + +Initialize DAL-A project intelligence for this repo by scanning conventions and docs, documenting flows, and generating `.github/copilot`, flow docs, and applicable hooks. + +## Decisions + +- 2026-04-18: Treat this as a first-time DAL-A setup because `.github/copilot/project-context.md` does not exist in the repo. + +## Changelog + +- 2026-04-18: Task created with confirmed goal; implementation not started yet. +- 2026-04-21: Inventoried `CLAUDE.md`, `README.md`, and `docs/TESTING.md`; confirmed a React-only frontend app with Firebase session sharing and no backend directory. +- 2026-04-21: Generated `.github/copilot/project-context.md`, `constraints.md`, `dependencies.md`, and `copilot-instructions.md`. +- 2026-04-21: Created repo-local runtime flow docs under `.github/skills/flows/` with Mermaid diagrams for bootstrap, scale, playback, sharing, video, and settings propagation. +- 2026-04-21: Added `.github/hooks/hooks.json` and hook scripts for constraint enforcement, dangerous command prompts, flow reminders, and constraint forwarding. +- 2026-04-21: Added a standalone `lint` script and direct `eslint` devDependency entry in `package.json`; executable lint validation remains blocked because this environment lacks installed dependencies and Yarn registry resolution failed. + +## Improvements Queued + +- None currently. diff --git a/.github/copilot/tasks/issue-350-order-of-notations/task.md b/.github/copilot/tasks/issue-350-order-of-notations/task.md new file mode 100644 index 00000000..518913d7 --- /dev/null +++ b/.github/copilot/tasks/issue-350-order-of-notations/task.md @@ -0,0 +1,48 @@ +--- +title: "Issue #350 order of notations" +slug: "issue-350-order-of-notations" +phase: "done" +goal: "Make notation layers stack bottom-to-top in the UI in the exact order of the Notation checkbox list, and keep that order stable across all interactions, without regressing other notation modes or accessibility." +current_action: "Task complete. Fix pushed to branch issue-350-order-of-notations-fix; PR #351 updated with new description." +--- + +Fix GitHub issue [#350](https://github.com/NYUMusEdLab/notio/issues/350). The reporter observed that notation stacking on the staff sometimes becomes reversed/random, with scale-step and chord-extension numbers ending up below note names. The expected behavior is a stable stacking order that matches the order of items in the Notation checkbox list, bottom-to-top. + +## Constraints + +- No regressions to other notation modes or accessibility. +- Must include automated regression tests. +- Scope: UI stacking order only (not an end-to-end re-architecture). +- This branch already has commit `fb80ceb` from a previous attempt; user chose "revert and replan", so the prior approach must be undone as part of this task. + +## Decisions + +| Date | Decision | Rationale | +| --- | --- | --- | +| 2026-04-21 | Revert prior fix commit `fb80ceb` and replan | Discovery showed it fixed the wrong seam (generic `ListCheckbox`) and missed the session-restore path | +| 2026-04-21 | Scope limited to UI stacking order | Keyboard label ordering out of scope unless proven same root cause | +| 2026-04-21 | Approach: Option 1 — state-boundary normalization in `WholeApp` | Single source of truth; fixes restored-array bug; small, test-friendly | +| 2026-04-21 | Visual direction: DOM order = canonical list order (Chord extensions on top, English at bottom) | `.noteWrapper` uses `flex-direction: column`, so DOM top = visual top; consistent with the issue reporter's expectation that numbers appear above note names | +| 2026-04-21 | Git cleanup via `git revert fb80ceb` (no force-push) | Preserves history; safe on already-pushed branch | + +## Roadmap + +- [x] Slice 1 — Revert `fb80ceb` and restore a clean baseline on branch `issue-350-order-of-notations-fix`. +- [x] Slice 2 — Add failing integration tests asserting rendered notation row order after menu writer and session-restore writer, with confirmed visual direction (Chord extensions on top, English at bottom). +- [x] Slice 3 — Introduce `normalizeNotationOrder` helper and apply it at every writer of `state.notation` in `WholeApp` (`handleChangeNotation`, session-restore, defaults); verify tests pass. +- [x] Slice 4 — Regression sweep: run full test suite + targeted a11y checks; update flow docs if behavior changed; push branch and update PR. + +## Changelog + +- 2026-04-21: Task created with confirmed goal and constraints; prior fix commit noted for revert. +- 2026-04-21: Discovery completed; identified `WholeApp` → `Keyboard` → `ColorKey` as the real ownership path and `WholeApp` session-restore as the missed seam. +- 2026-04-21: Roadmap confirmed (Option 1 — state-boundary normalization); visual direction and git cleanup strategy locked in. +- 2026-04-21: Slice 1 complete. Committed DAL-A setup (2348949), rewrote task.md (92a05bb), and reverted fb80ceb via `git revert` (72e321b). `src/components/form/ListCheckbox.js` and deletion of `src/__test__/ListCheckbox.order.test.js` now diff-clean vs `origin/master`. Branch is 3 commits ahead of master; origin PR not yet updated. +- 2026-04-21: Visual direction disambiguated with product: Chord extensions on top, English at bottom; DOM order = canonical order. +- 2026-04-21: Slice 2 complete. Added `src/__integration__/notation-order.test.js` with two failing tests covering the menu writer and session-restore writer paths. Both fail on current baseline with the expected "order mirrors scrambled input" diff. Test runtime ~4s (within integration-suite budget). +- 2026-04-21: Slice 3 complete. Added `src/Model/normalizeNotationOrder.js` helper and wired it into `handleChangeNotation`, `openSavedSession`, and a new `componentDidUpdate` safety net in `WholeApp.js` (Approach 3 — defense in depth). Both Slice 2 tests now pass; adjacent integration suites (34 tests) still pass. +- 2026-04-21: Slice 4 complete. Full test suite ran: 301/306 pass. The 5 failures (`notation-regression.test.js`, `relative-notenames.test.js`, `CustomVideoPlayer.integration.test.js`) were verified to fail identically on `origin/master` — pre-existing, unrelated to this fix. Updated `.github/skills/flows/settings-propagation.md` with the new canonical-order invariant. Pushed 7 commits to `origin/issue-350-order-of-notations-fix` (fast-forward, no force-push). Updated PR #351 body to reflect the new approach. + +## Improvements Queued + +- None yet. diff --git a/.github/hooks/hooks.json b/.github/hooks/hooks.json new file mode 100644 index 00000000..d689c384 --- /dev/null +++ b/.github/hooks/hooks.json @@ -0,0 +1,34 @@ +{ + "hooks": { + "PreToolUse": [ + { + "type": "command", + "command": "python .github/hooks/scripts/constraint-guard.py", + "windows": "python .github/hooks/scripts/constraint-guard.py", + "timeout": 10 + }, + { + "type": "command", + "command": "python .github/hooks/scripts/dangerous-command-guard.py", + "windows": "python .github/hooks/scripts/dangerous-command-guard.py", + "timeout": 10 + } + ], + "PostToolUse": [ + { + "type": "command", + "command": "python .github/hooks/scripts/flow-verification-reminder.py", + "windows": "python .github/hooks/scripts/flow-verification-reminder.py", + "timeout": 10 + } + ], + "SubagentStart": [ + { + "type": "command", + "command": "python .github/hooks/scripts/constraint-forwarding.py", + "windows": "python .github/hooks/scripts/constraint-forwarding.py", + "timeout": 10 + } + ] + } +} \ No newline at end of file diff --git a/.github/hooks/scripts/constraint-forwarding.py b/.github/hooks/scripts/constraint-forwarding.py new file mode 100644 index 00000000..0ffd81f3 --- /dev/null +++ b/.github/hooks/scripts/constraint-forwarding.py @@ -0,0 +1,110 @@ +""" +Constraint Forwarding Hook — SubagentStart context injection. + +When a subagent starts, it has a fresh context with no knowledge of +project constraints. This hook reads constraints.md and injects a +compact summary as a system message. +""" + +import json +import re +import sys +from pathlib import Path + + +def locate_constraints_in_ancestor_directories(): + """Walk up from cwd to find .github/copilot/constraints.md.""" + current_directory = Path.cwd() + for ancestor in [current_directory, *current_directory.parents]: + constraints_path = ancestor / ".github" / "copilot" / "constraints.md" + if constraints_path.exists(): + return constraints_path + return None + + +def is_meaningful_line(line): + stripped_line = line.strip() + return bool(stripped_line) and not stripped_line.startswith(" B["WholeApp or WholeAppWrapper"] + end + subgraph startup["Bootstrap"] + B --> C["WholeApp componentDidMount"] + C --> D{"sessionId present?"} + D -->|no| E["set loading false"] + D -->|yes| F["openSavedSession"] + F --> G["hydrate state from Firestore"] + G --> E + end + E --> H["Render keyboard, menu, overlays"] +``` + +| Step | File | Function | +| --- | --- | --- | +| Route selection | `src/index.js` | `root.render` | +| Shared route handoff | `src/WholeAppWrapper.js` | `WholeAppWrapper` | +| App mount | `src/WholeApp.js` | `componentDidMount` | +| Session fetch | `src/WholeApp.js` | `openSavedSession` | +| Persistence client | `src/Firebase.js` | `db.collection(...).doc(...).get` | + +## Invariants + +- `loading` must eventually become `false` for the app to render beyond the loading screen. +- Shared-session hydration must preserve custom scale registration before the main UI renders. +- The `/shared/:sessionId` route is the only session-restore entry path. + +## Dependencies + +- Firebase Firestore +- settings-propagation +- session-share diff --git a/.github/skills/flows/note-playback.md b/.github/skills/flows/note-playback.md new file mode 100644 index 00000000..214a9df0 --- /dev/null +++ b/.github/skills/flows/note-playback.md @@ -0,0 +1,45 @@ +--- +flow: "note-playback" +entry: "src/components/keyboard/Keyboard.js:handleKeyDown" +exit: "A note is played, visually highlighted, and released" +--- + +# note-playback + +[← Flow Catalog](./SKILL.md) + +```mermaid +flowchart LR + subgraph input["Input Paths"] + A["QWERTY, mouse, touch, or focused key activation"] --> B["noteOnHandler or playNote"] + end + subgraph audio["Playback"] + B --> C["SoundMaker startSound"] + C --> D["Tone.js or SoundFont adapter"] + end + subgraph ui["Feedback"] + B --> E["Highlight active key"] + D --> F["Audible note"] + E --> G["Release path removes highlight"] + end +``` + +| Step | File | Function | +| --- | --- | --- | +| Keyboard input handling | `src/components/keyboard/Keyboard.js` | `handleKeyDown` | +| Note activation guard | `src/components/keyboard/Keyboard.js` | `noteOnHandler` | +| Audio facade | `src/Model/SoundMaker.js` | `startSound` | +| Tone adapter | `src/Model/Adapters/Adapter_Tonejs_to_SoundMaker.js` | `keyDown` | +| Focus-key activation | `src/components/keyboard/Key.js` | `handleKeyDown` | + +## Invariants + +- A note should not be double-triggered while it remains active. +- Audio resume must happen from a user interaction path before playback in browsers that gate AudioContext startup. +- Key highlight and release behavior should stay aligned with note start and stop behavior. + +## Dependencies + +- scale-change-pipeline +- Accessibility tests under `src/__integration__/accessibility/` +- Audio adapters under `src/Model/Adapters/` diff --git a/.github/skills/flows/scale-change-pipeline.md b/.github/skills/flows/scale-change-pipeline.md new file mode 100644 index 00000000..9d318a88 --- /dev/null +++ b/.github/skills/flows/scale-change-pipeline.md @@ -0,0 +1,45 @@ +--- +flow: "scale-change-pipeline" +entry: "src/WholeApp.js:handleSelectScale" +exit: "Keyboard labels, colors, and in-scale state update for the selected recipe" +--- + +# scale-change-pipeline + +[← Flow Catalog](./SKILL.md) + +```mermaid +flowchart LR + subgraph menu["Selection"] + A["TopMenu scale or custom scale action"] --> B["WholeApp state update"] + end + subgraph domain["Domain Build"] + B --> C["Keyboard componentDidUpdate"] + C --> D["new MusicScale(...)" ] + D --> E["BuildExtendedScaleToneNames"] + end + subgraph render["Render"] + E --> F["Map scale to keyboard layout"] + F --> G["Render Key and ColorKey state"] + end +``` + +| Step | File | Function | +| --- | --- | --- | +| Scale selection | `src/WholeApp.js` | `handleSelectScale` | +| Custom scale insertion | `src/WholeApp.js` | `handleChangeCustomScale` | +| Keyboard rebuild | `src/components/keyboard/Keyboard.js` | `componentDidUpdate` | +| Domain generation | `src/Model/MusicScale.js` | `init` | +| Relative notation mapping | `src/Model/MusicScale.js` | `makeRelativeScaleSyllables` | + +## Invariants + +- Relative notation must stay key-independent for the same scale recipe across all roots. +- Changes to one notation mode must not corrupt the others. +- Missing scale recipes should be treated as defects, not normal fallbacks. + +## Dependencies + +- settings-propagation +- note-playback +- Relative-notation regression and integration tests under `src/__test__/` and `src/__integration__/` diff --git a/.github/skills/flows/session-share.md b/.github/skills/flows/session-share.md new file mode 100644 index 00000000..76a40309 --- /dev/null +++ b/.github/skills/flows/session-share.md @@ -0,0 +1,43 @@ +--- +flow: "session-share" +entry: "src/components/menu/ShareLink.js:onClick" +exit: "A shared session URL is created and can later restore the same app state" +--- + +# session-share + +[← Flow Catalog](./SKILL.md) + +```mermaid +flowchart LR + subgraph create["Create Share Link"] + A["ShareLink click"] --> B["saveSessionToDB"] + B --> C["Firestore add session"] + C --> D["Build /shared/:sessionId URL"] + end + subgraph restore["Restore"] + D --> E["User opens shared route"] + E --> F["WholeAppWrapper passes sessionId"] + F --> G["openSavedSession hydrates state"] + end +``` + +| Step | File | Function | +| --- | --- | --- | +| Share action | `src/components/menu/ShareLink.js` | `onClick` | +| Session save | `src/WholeApp.js` | `saveSessionToDB` | +| Firebase write | `src/Firebase.js` | `db.collection(...).add` | +| Shared route handoff | `src/WholeAppWrapper.js` | `WholeAppWrapper` | +| Session restore | `src/WholeApp.js` | `openSavedSession` | + +## Invariants + +- Saved and restored state fields must stay in sync when new session-relevant state is introduced. +- Share creation should never silently produce a broken `/shared/undefined` URL. +- Session restore must re-register custom scales before rendering dependent UI. + +## Dependencies + +- app-bootstrap +- Firebase Firestore +- Share menu components under `src/components/menu/` diff --git a/.github/skills/flows/settings-propagation.md b/.github/skills/flows/settings-propagation.md new file mode 100644 index 00000000..db323a63 --- /dev/null +++ b/.github/skills/flows/settings-propagation.md @@ -0,0 +1,49 @@ +--- +flow: "settings-propagation" +entry: "src/components/menu/TopMenu.js" +exit: "Menu actions fan out through WholeApp into keyboard, notation, audio, and overlays" +--- + +# settings-propagation + +[← Flow Catalog](./SKILL.md) + +```mermaid +flowchart LR + subgraph controls["Menu Controls"] + A["TopMenu interactions"] --> B["WholeApp handlers"] + end + subgraph state["State Fan-out"] + B --> C["WholeApp setState"] + C --> D["Keyboard props update"] + C --> E["Overlay and menu props update"] + end + subgraph outcome["Visible Results"] + D --> F["Notation, sound, root, scale, clef changes"] + E --> G["Share, help, and video UI changes"] + end +``` + +| Step | File | Function | +| --- | --- | --- | +| Menu rendering | `src/components/menu/TopMenu.js` | `TopMenu` | +| Root change | `src/WholeApp.js` | `handleChangeRoot` | +| Notation change | `src/WholeApp.js` | `handleChangeNotation` | +| Notation canonicalization | `src/Model/normalizeNotationOrder.js` | `normalizeNotationOrder` | +| Notation invariant enforcement | `src/WholeApp.js` | `componentDidUpdate` | +| Sound change | `src/WholeApp.js` | `handleChangeSound` | +| Video tab change | `src/WholeApp.js` | `handleChangeActiveVideoTab` | + +## Invariants + +- Menu interactions should update only the intended slices of app state. +- Keyboard, audio, and overlay behavior should stay synchronized with the active settings. +- Accessibility affordances in menu navigation must remain intact. +- **`state.notation` is always in canonical Notation-menu order** (`["Colors", "Chord extensions", "Scale Steps", "Relative", "Romance", "German", "English"]`). This invariant is enforced at every known writer (`handleChangeNotation`, `openSavedSession`) and guarded by `componentDidUpdate` for any other writer. Enforced since issue #350. + +## Dependencies + +- scale-change-pipeline +- note-playback +- video-overlay +- Accessibility menu-navigation tests diff --git a/.github/skills/flows/video-overlay.md b/.github/skills/flows/video-overlay.md new file mode 100644 index 00000000..4d03b3dd --- /dev/null +++ b/.github/skills/flows/video-overlay.md @@ -0,0 +1,47 @@ +--- +flow: "video-overlay" +entry: "src/components/menu/VideoButton.js:handleShow" +exit: "The custom video player opens, updates URL state, and returns focus cleanly on close" +--- + +# video-overlay + +[← Flow Catalog](./SKILL.md) + +```mermaid +flowchart LR + subgraph open["Open Overlay"] + A["VideoButton action"] --> B["Toggle video visibility"] + B --> C["Render CustomVideoPlayer"] + end + subgraph player["Player State"] + C --> D["Load current or default URL"] + D --> E["Render ReactPlayer or iframe"] + C --> F["Tab and URL updates"] + end + subgraph close["Close"] + E --> G["Overlay closes"] + F --> G + G --> H["Focus returns to trigger"] + end +``` + +| Step | File | Function | +| --- | --- | --- | +| Open or close trigger | `src/components/menu/VideoButton.js` | `handleShow` | +| Visibility toggle | `src/WholeApp.js` | `handleChangeVideoVisibility` | +| URL state update | `src/WholeApp.js` | `handleChangeVideoUrl` | +| Reset URL | `src/WholeApp.js` | `handleResetVideoUrl` | +| Custom player submit | `src/components/menu/CustomVideoPlayer.js` | `handleSubmit` | + +## Invariants + +- Closing the overlay should return focus to the trigger control. +- Only valid supported URL forms should reach embedded player surfaces. +- Reset should restore the preserved baseline video URL rather than an arbitrary prior value. + +## Dependencies + +- settings-propagation +- `src/data/config.js` +- Integration tests for the custom video player diff --git a/package.json b/package.json index cbb42579..2a4fd56a 100644 --- a/package.json +++ b/package.json @@ -29,6 +29,7 @@ "scripts": { "start": "react-scripts start", "build": "CI=false react-scripts build && cp netlify.toml build/netlify.toml", + "lint": "eslint src --ext .js,.jsx", "test": "react-scripts test --transformIgnorePatterns \"node_modules/(?!(vexflow|gsap)/)\" --watchAll", "test-ci": "react-scripts test --transformIgnorePatterns \"node_modules/(?!(vexflow|@tonejs/piano|gsap)/)\" --ci", "test:e2e": "playwright test", @@ -70,6 +71,7 @@ "@playwright/test": "^1.56.1", "@testing-library/jest-dom": "^5.16.5", "babel-jest": "^29.0.3", + "eslint": "^8.3.0", "jest": "^29.0.3", "jest-axe": "^10.0.0", "jest-html-reporter": "^3.7.0", diff --git a/src/Model/normalizeNotationOrder.js b/src/Model/normalizeNotationOrder.js new file mode 100644 index 00000000..580cd244 --- /dev/null +++ b/src/Model/normalizeNotationOrder.js @@ -0,0 +1,49 @@ +/** + * normalizeNotationOrder + * + * Returns a new array containing the same notation values as the input, but + * re-ordered to the canonical Notation-menu order. Any "Colors" entry is + * preserved at the front of the array; "Colors" is a non-rendering option + * (the `case "Colors"` branch in `Keyboard` is a no-op) kept in + * `state.notation` for backward compatibility with stored sessions and + * defaults. Unknown values are preserved at the end to avoid silently + * dropping legacy or forward-compatible notation identifiers. + * + * The canonical order must mirror `notations` in + * `src/components/menu/Notation.js`, prepended with "Colors". Keep these in + * sync when notation options change. + * + * Contract: + * - Idempotent (content equality): re-applying the function produces an + * array deep-equal to the previous result. The returned array is always + * a new instance, so referential equality (`===`) is not guaranteed. + * - Never mutates the input array. + * - Non-array inputs are returned unchanged (defensive for session-restore + * payloads that might be malformed). + */ + +export const CANONICAL_NOTATION_ORDER = [ + "Colors", + "Chord extensions", + "Scale Steps", + "Relative", + "Romance", + "German", + "English", +]; + +const CANONICAL_SET = new Set(CANONICAL_NOTATION_ORDER); + +export function normalizeNotationOrder(notation) { + if (!Array.isArray(notation)) { + return notation; + } + const selected = new Set(notation); + const known = CANONICAL_NOTATION_ORDER.filter((name) => selected.has(name)); + const unknown = notation.filter( + (name) => !CANONICAL_SET.has(name) + ); + return [...known, ...unknown]; +} + +export default normalizeNotationOrder; diff --git a/src/WholeApp.js b/src/WholeApp.js index 0c86c9ac..2d2b2f61 100644 --- a/src/WholeApp.js +++ b/src/WholeApp.js @@ -10,6 +10,7 @@ import scales from "./data/scalesObj"; import { MobileView } from 'react-device-detect'; import Popup from 'reactjs-popup'; import SoundLibraryNames from "data/TonejsSoundNames"; +import normalizeNotationOrder from "./Model/normalizeNotationOrder"; // TODO:to meet the requirements for router-dom v6 useParam hook can not be used in class Components and props.match.params only works in v5: //This is using a wrapper function for wholeApp because wholeApp is a class and not a functional component, REWRITE wholeApp to a const wholeApp =()=>{...} @@ -164,7 +165,10 @@ class WholeApp extends Component { handleChangeNotation = (selectedNotation) => { // console.log(selectedNotation + " Notation selected"); - this.setState({ notation: selectedNotation }); + // Normalize to canonical Notation-menu order so the rendered stack in + // Keyboard/ColorKey is stable regardless of the order in which + // ListCheckbox emits selected values (issue #350). + this.setState({ notation: normalizeNotationOrder(selectedNotation) }); }; handleChangeCustomScale = (customScaleName, customsteps, customNumbers, firstRun = false) => { @@ -326,7 +330,7 @@ class WholeApp extends Component { scale: result.scale, scaleObject: result.scaleObject, baseNote: result.baseNote, - notation: result.notation, + notation: normalizeNotationOrder(result.notation), instrumentSound: result.instrumentSound, pianoOn: result.pianoOn, extendedKeyboard: result.extendedKeyboard, @@ -396,6 +400,34 @@ class WholeApp extends Component { */ } + /** + * Safety net for issue #350: enforce that `state.notation` is always in + * canonical Notation-menu order, no matter which writer produced it. + * + * Named writers (`handleChangeNotation`, `openSavedSession`) already pass + * through `normalizeNotationOrder` eagerly, so the common path renders in + * canonical order on the first frame. This hook guards against any other + * caller (direct setState, future writers, or legacy session shapes) that + * may bypass the named writers, making canonical order a true invariant of + * `state.notation` rather than a convention. + */ + componentDidUpdate(prevProps, prevState) { + if (prevState.notation === this.state.notation) { + return; + } + const current = this.state.notation; + if (!Array.isArray(current)) { + return; + } + const normalized = normalizeNotationOrder(current); + const isAlreadyCanonical = + normalized.length === current.length && + normalized.every((value, index) => value === current[index]); + if (!isAlreadyCanonical) { + this.setState({ notation: normalized }); + } + } + toggleMenu = () => { this.setState({ menuOpen: !this.state.menuOpen }); }; diff --git a/src/__integration__/notation-order.test.js b/src/__integration__/notation-order.test.js new file mode 100644 index 00000000..5d5674b6 --- /dev/null +++ b/src/__integration__/notation-order.test.js @@ -0,0 +1,208 @@ +/** + * Integration Tests: Notation Stacking Order (Issue #350) + * + * Purpose: Lock the visual contract that the notation layers rendered on each + * key stack in the canonical Notation-menu order regardless of the order in + * which the notation values are written into WholeApp state. + * + * Constitution v2.0.0: Integration test (PRIMARY - 60-70% of suite) + * + * Canonical order (from src/components/menu/Notation.js `notations`): + * "Chord extensions", "Scale Steps", "Relative", "Romance", "German", "English" + * + * Corresponding DOM class suffixes rendered by ColorKey: + * Extension, Step, Relative, Romance, German, English + * + * Visual direction (confirmed with product): + * Top of visual stack = FIRST in canonical list (Chord extensions on top, + * English at bottom). DOM order equals canonical order because + * `.noteWrapper` uses `flex-direction: column` (DOM top = visual top). + * + * These tests intentionally feed scrambled notation arrays through two + * distinct writer seams: + * 1. Menu writer: instance.handleChangeNotation([...]) + * 2. Direct state write (used by openSavedSession session-restore): + * instance.setState({ notation: [...] }) + * + * Both should still produce a canonically-ordered DOM stack once the + * state-boundary normalization fix lands (Slice 3). Today both fail because + * the rendered order is taken literally from state.notation. + */ + +import React from 'react'; +import { render, screen, waitFor, act } from '@testing-library/react'; +import WholeApp from '../WholeApp'; + +// Mock SoundMaker to avoid audio context issues in tests +jest.mock('../Model/SoundMaker'); + +// Mock react-dom createPortal for tooltip/modal rendering +jest.mock('react-dom', () => ({ + ...jest.requireActual('react-dom'), + createPortal: (element) => element, +})); + +// Mock VexFlow for MusicalStaff rendering +jest.mock('vexflow', () => { + const mockContext = { + setViewBox: jest.fn(), + setFont: jest.fn(), + setStrokeStyle: jest.fn(), + setFillStyle: jest.fn(), + clear: jest.fn(), + }; + class MockRenderer { + constructor() { + this.resize = jest.fn(); + this.getContext = jest.fn().mockReturnValue(mockContext); + } + static Backends = { SVG: 1, CANVAS: 2 }; + } + class MockVoice { + constructor(cfg) { + this.config = cfg; + this.addTickables = jest.fn().mockReturnThis(); + this.draw = jest.fn(); + } + } + class MockFormatter { + constructor() { + this.joinVoices = jest.fn().mockReturnThis(); + this.format = jest.fn().mockReturnThis(); + } + } + const mockStaveNote = jest.fn().mockImplementation((config) => ({ + ...config, + addModifier: jest.fn().mockReturnThis(), + })); + class MockStave { + constructor() { + this.setBegBarType = jest.fn().mockReturnThis(); + this.setEndBarType = jest.fn().mockReturnThis(); + this.setContext = jest.fn().mockReturnThis(); + this.draw = jest.fn().mockReturnThis(); + this.addClef = jest.fn().mockReturnThis(); + } + } + const Flow = { + Renderer: MockRenderer, + Stave: MockStave, + StaveNote: mockStaveNote, + Voice: MockVoice, + Formatter: MockFormatter, + Accidental: jest.fn(), + Barline: { type: { NONE: 1, SINGLE: 2, DOUBLE: 3, END: 4 } }, + }; + return { Flow, default: { Flow } }; +}); + +// Canonical DOM-class suffix order corresponding to the canonical notation +// list. ColorKey maps "Scale Steps" -> "Step" and "Chord extensions" -> +// "Extension" when it builds className=`noteWrapper--${item.key}`. +const CANONICAL_CLASS_SUFFIXES = [ + 'Extension', + 'Step', + 'Relative', + 'Romance', + 'German', + 'English', +]; + +/** + * Return the ordered list of notation-class suffixes rendered inside the + * first ColorKey in the DOM, in DOM order (= visual top-to-bottom). + */ +const readNotationRowOrder = (container) => { + // Query any element whose class contains noteWrapper--X where X is one of + // the canonical suffixes. Use one of the in-scale ColorKeys; the first one + // in the DOM is representative since all in-scale ColorKeys share the + // same notation array. + const firstColorKey = container.querySelector('[data-testid^="ColorKey:"] .noteWrapper--Extension, [data-testid^="ColorKey:"] .noteWrapper--Step, [data-testid^="ColorKey:"] .noteWrapper--Relative, [data-testid^="ColorKey:"] .noteWrapper--Romance, [data-testid^="ColorKey:"] .noteWrapper--German, [data-testid^="ColorKey:"] .noteWrapper--English'); + if (!firstColorKey) { + return []; + } + // Walk up to the containing ColorKey then collect all notation rows inside it. + const colorKey = firstColorKey.closest('[data-testid^="ColorKey:"]'); + const rows = Array.from( + colorKey.querySelectorAll('[class*="noteWrapper--"]') + ); + return rows + .map((el) => { + const match = Array.from(el.classList).find((c) => + c.startsWith('noteWrapper--') + ); + return match ? match.replace('noteWrapper--', '') : null; + }) + .filter((s) => CANONICAL_CLASS_SUFFIXES.includes(s)); +}; + +describe('Integration Test: Notation stacking order (Issue #350)', () => { + let appRef; + + beforeEach(() => { + jest.clearAllMocks(); + appRef = React.createRef(); + }); + + it('renders notation rows in canonical order after menu writer receives a scrambled array', async () => { + const { container } = render(); + + await waitFor(() => { + expect(screen.getByTestId('Keyboard')).toBeInTheDocument(); + }); + + // Menu writer path: ListCheckbox callback may emit values in toggle-history + // order, not canonical order. Simulate that by handing the writer a + // scrambled subset of all notations. + const scrambled = [ + 'English', + 'Chord extensions', + 'Relative', + 'Scale Steps', + 'German', + 'Romance', + ]; + + await act(async () => { + appRef.current.handleChangeNotation(scrambled); + }); + + await waitFor(() => { + expect(readNotationRowOrder(container).length).toBeGreaterThan(0); + }); + + const domOrder = readNotationRowOrder(container); + expect(domOrder).toEqual(CANONICAL_CLASS_SUFFIXES); + }); + + it('renders notation rows in canonical order after a direct state write (session-restore path)', async () => { + const { container } = render(); + + await waitFor(() => { + expect(screen.getByTestId('Keyboard')).toBeInTheDocument(); + }); + + // Direct state write simulates WholeApp.openSavedSession, which restores + // state.notation with result.notation verbatim from a Firestore document. + // Legacy sessions can carry arrays in any order. + const legacyStoredOrder = [ + 'English', + 'German', + 'Romance', + 'Relative', + 'Scale Steps', + 'Chord extensions', + ]; + + await act(async () => { + appRef.current.setState({ notation: legacyStoredOrder }); + }); + + await waitFor(() => { + expect(readNotationRowOrder(container).length).toBeGreaterThan(0); + }); + + const domOrder = readNotationRowOrder(container); + expect(domOrder).toEqual(CANONICAL_CLASS_SUFFIXES); + }); +});