fix(editor): preserve the full computed anchor set when authoring one

[ChrisBeWithYou]

Summary

• On the anchor lane, the backend's computed/source anchors are shown until you author your own; the backend treats a non-empty anchors_user as the complete authored list (empty = recompute on save).
• The bug: interacting with a single computed anchor (click / right-click edit / drag) promoted only that one anchor into anchors_user, and adding your first anchor in empty lane space pushed only the new one — either way anchors_user became a one-element list, so every other computed anchor was silently dropped on the next save (and disappeared from the lane).
• The fix: both authoring entry points now materialize the whole computed set into anchors_user on that first interaction, as a single undoable PromoteAnchorsCmd (undo returns to the recompute-on-save fallback). Only the anchor you actually edit changes.
• The anchor edit dialog now labels the value as the hand-position fret (index finger) and the width as hand span, so "fret" no longer reads as an arbitrary number.
• Deferred to a follow-up: a persistent "reset to computed anchors" action — undo already reverts a promotion, and a durable reset needs the backend's absent-vs-empty save semantics handled explicitly.

Tests

node tests/anchor_authoring.test.js → 5 passed. Full JS suite: 13 files, 0 failures.

New tests/anchor_authoring.test.js brace-extracts the real anchor source (screen.js is one IIFE) and pins:

• promoting one computed anchor seeds the full set, not a lone anchor;
• adding a new anchor seeds the fallback set first, preserving the computed anchors;
• undo restores the empty → recompute fallback;
• already-authored markers pass through untouched.

Confirmed the regression test fails against the pre-fix code (it collapses 3 computed anchors to 1).

Implements the anchor-authoring-semantics fix from the song editor design charrette (the highest-risk bug: fallback promotion collapsing a full computed anchor set).

Summary by CodeRabbit

• Bug Fixes
  • Editing (click, context-menu edit, or drag) no longer overwrites or removes other computed anchors; the full computed set is preserved and promoted correctly.
  • Insertions on empty space now convert computed anchors into authored anchors in one undoable step, keeping the editor state consistent.
• New Features
  • Updated anchor edit labels to “Hand-position fret — index finger” and “Hand span”.
• Tests
  • Added regression tests covering computed vs authored anchor promotion and undo/rollback behavior.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR changes computed-anchor editing so the first authoring action promotes the full computed set into anchors_user with undo support, updates anchor edit labels, adds regression tests, and records the fix in the changelog.

Changes

Anchor Promotion Fix

Layer / File(s)	Summary
PromoteAnchorsCmd implementation screen.js	_promoteAnchor now uses PromoteAnchorsCmd for computed anchors, and the new command copies the full fallback set into arr.anchors_user, returns the authored copy for the clicked marker, and rolls back the seeded copies.
Promotion wiring into edit and insert flows screen.js	Empty-area anchor insertion now chooses between AddAnchorCmd and PromoteAnchorsCmd based on whether anchors_user already exists, seeding the full fallback set on the first authored insert.
Dialog label wording updates screen.js	The anchor edit prompts now use the updated fret and span labels.
Regression tests for anchor promotion tests/anchor_authoring.test.js	A sandboxed test harness extracts internal helpers from screen.js and verifies promotion, undo, idempotency, fallback seeding, and authored add behavior.
Changelog entry CHANGELOG.md	The changelog records the computed-anchor fix, dialog label changes, and test coverage.

Estimated code review effort: 3 (Moderate) | ~25 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 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 editor change: preserving the full computed anchor set when a user authors an anchor.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.

✨ 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 fix/anchor-authoring-semantics

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

screen.js (1)

10975-10984: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Collapse fallback promotion and insert into one undo step (screen.js:10975-10981)
_promoteAutoAnchorsIfNeeded(arr) adds a PromoteAnchorsCmd before AddAnchorCmd, so the first authored anchor on fallback still takes two undo actions to return to recompute-on-save. If the “one undoable step” contract is intended, wrap both in a single history entry.

🤖 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 `@screen.js` around lines 10975 - 10984, The first authored anchor on fallback
currently creates two separate history actions because
_promoteAutoAnchorsIfNeeded(arr) pushes a PromoteAnchorsCmd before the
AddAnchorCmd. Update the anchor insertion flow around the existing
_promoteAutoAnchorsIfNeeded, AddAnchorCmd, and S.history.exec usage so promotion
and insert are recorded as one undoable step, preserving the one-step undo
contract. Ensure the combined action still sets S.anchorSel, calls draw(), and
returns true as before.

🧹 Nitpick comments (1)

tests/anchor_authoring.test.js (1)

25-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

Brace-matching extraction is fragile against future screen.js refactors.

Pulling functions/classes out of screen.js via indexOf + brace-matching works today, but silently breaks (throws decl not found) if a function is renamed, reformatted, or converted to an arrow function/method shorthand later. Since screen.js has no module boundaries to import from directly, this is a reasonable workaround, but worth flagging as a long-term maintenance cost.

Consider exposing a small internal test-only export (e.g., attaching these functions to a module.exports guarded by typeof module !== 'undefined') if screen.js is refactored in the future, to make this test harness robust to reformatting.

🤖 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/anchor_authoring.test.js` around lines 25 - 48, The test harness in
extractDecl/new Function is brittle because it depends on exact source text for
_readAnchorSnapshot, _ensureAnchors, _bumpAnchorsDirty, _anchorsAreDirty,
PromoteAnchorsCmd, AddAnchorCmd, _promoteAnchor, and
_promoteAutoAnchorsIfNeeded. Update the anchor authoring tests to use a more
stable internal test-only export from screen.js (or another explicit test hook)
instead of indexOf/brace-matching, so future refactors like renames, formatting
changes, or method-shorthand conversions do not break the suite.

🤖 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.

Outside diff comments:
In `@screen.js`:
- Around line 10975-10984: The first authored anchor on fallback currently
creates two separate history actions because _promoteAutoAnchorsIfNeeded(arr)
pushes a PromoteAnchorsCmd before the AddAnchorCmd. Update the anchor insertion
flow around the existing _promoteAutoAnchorsIfNeeded, AddAnchorCmd, and
S.history.exec usage so promotion and insert are recorded as one undoable step,
preserving the one-step undo contract. Ensure the combined action still sets
S.anchorSel, calls draw(), and returns true as before.

---

Nitpick comments:
In `@tests/anchor_authoring.test.js`:
- Around line 25-48: The test harness in extractDecl/new Function is brittle
because it depends on exact source text for _readAnchorSnapshot, _ensureAnchors,
_bumpAnchorsDirty, _anchorsAreDirty, PromoteAnchorsCmd, AddAnchorCmd,
_promoteAnchor, and _promoteAutoAnchorsIfNeeded. Update the anchor authoring
tests to use a more stable internal test-only export from screen.js (or another
explicit test hook) instead of indexOf/brace-matching, so future refactors like
renames, formatting changes, or method-shorthand conversions do not break the
suite.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 2e462095-9aad-49b2-ade1-4a2abcba2ad2

📥 Commits

Reviewing files that changed from the base of the PR and between 5239d47 and 1c2e0bf.

📒 Files selected for processing (3)

• CHANGELOG.md
• screen.js
• tests/anchor_authoring.test.js

[byrongamatos]

Review + fix pass (Claude Code): pushed a fix for an undo-semantics bug.

Bug: the empty-space first anchor insert (while on the computed/source fallback) ran two history commands — PromoteAnchorsCmd (seed the whole computed set) then AddAnchorCmd (the new anchor). A single Ctrl+Z rolled back only the add, leaving anchors_user seeded and _anchorEditCount at 1 — so the save path (_anchorsAreDirty gate) still shipped the promoted set as authored and the arrangement never returned to recompute-on-save fallback. This contradicted the PR's own 'single undoable' intent (the promote path was already one command; the insert path wasn't).

Fix: fold seed + insert into a single PromoteAnchorsCmd (optional new-anchor arg, seeded by reference so selection/drag stay live); already-authored inserts remain a plain single AddAnchorCmd. Removed the now-unused _promoteAutoAnchorsIfNeeded. Updated the tests to pin one-command insert + one-undo revert. All 13 JS test files green; the anchor P2 is cleared on re-review.

(Note: the reference-identity of the clicked marker to an arr.anchors member — via _readAnchorSnapshot returning arr.anchors directly on fallback — means the 'defensive lone copy' branch can't create a duplicate. Verified.)

[coderabbitai]

🧹 Nitpick comments (1)

tests/anchor_authoring.test.js (1)

18-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

Sandbox extraction technique is fragile to future refactors, but acceptable for now.

Regex/brace-matching extraction of named declarations from screen.js source, then eval'ing them via new Function, works but silently produces confusing runtime errors (e.g. PromoteAnchorsCmd is not a constructor) if a target function/class is ever renamed or restructured in screen.js, rather than a clear "declaration not found" failure. This is a test-only maintenance risk, not the security issue static analysis suggests (input is the project's own source, not external/untrusted data).

Consider having extractDecl() throw immediately if a declaration isn't found, to fail fast with an actionable message during refactors.

🤖 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/anchor_authoring.test.js` around lines 18 - 47, The test’s declaration
extraction in extractDecl() and the sandbox setup should fail fast when a target
symbol is missing instead of later producing confusing constructor/runtime
errors. Update the logic around extractDecl('function _readAnchorSnapshot('),
extractDecl('class PromoteAnchorsCmd '), and the other listed symbols so that
any rename or refactor in screen.js immediately throws a clear “declaration not
found” style error with the missing header. Keep the behavior test-only and
focused on making refactor failures actionable during sandbox construction.

🤖 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.

Nitpick comments:
In `@tests/anchor_authoring.test.js`:
- Around line 18-47: The test’s declaration extraction in extractDecl() and the
sandbox setup should fail fast when a target symbol is missing instead of later
producing confusing constructor/runtime errors. Update the logic around
extractDecl('function _readAnchorSnapshot('), extractDecl('class
PromoteAnchorsCmd '), and the other listed symbols so that any rename or
refactor in screen.js immediately throws a clear “declaration not found” style
error with the missing header. Keep the behavior test-only and focused on making
refactor failures actionable during sandbox construction.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 203de100-e4f9-4627-ac6f-07cdfadec95a

📥 Commits

Reviewing files that changed from the base of the PR and between 1c2e0bf and 4cb8634.

📒 Files selected for processing (2)

• screen.js
• tests/anchor_authoring.test.js