refactor(editor): move suggested-mark persistence and the roll lock notice (R2, step 13)

[byrongamatos]

Step 13. main.js 18,671 → 18,565. Two small moves, each to where the code already belonged — chosen to shrink the EditHistory seam before extracting it.

What moved

• _suggestedCount / _saveSuggestedMarks / _restoreSuggestedMarks + the @pure:suggest-marks-persist pures → src/notes.js, next to the WeakSet they read. They already had zero dependencies on the rest of main.js.
• _rollLockNotice → src/keys.js, next to _rollReadOnly, the predicate that decides when it fires. keys.js now imports setStatus — which is exactly why ui.js got its own module in step 10.

Also deletes a comment main.js had been carrying since step 9b, truncated mid-sentence when the WeakSet moved out:

// (an extra field would vanish). A WeakSet is invisible to serialization by

Its full rationale lives in notes.js.

Tests

suggest_position_persist loses its sandbox entirely. Every symbol it exercises is now an import, so it drives the real S with a globalThis.localStorage stub.

suggest_position_wiring's env now shares the real S with the imported _suggestedCount. This mattered: a private S would have left the count reading an arrangement the sliced commands never touched — green, and meaningless. That's the same trap I flagged on #151, arriving from the other direction.

The lint gate paid for itself, twice

• A missing import: _suggestedStorageKeyPure, still used by the save-as rename path in main.js.
• An unused import: _suggestedNotes, whose readers all moved.

Both are exactly the class of bug no-undef/no-unused-vars were added for on #159, and neither would have failed a test.

Why these two, together

EditHistory is 112 lines with a single, already-typeof-guarded S reference — but it's the keystone of the 1,536-line Undo/Redo section. Its coupling to main.js is now three symbols, down from four: _historyEnsureArr, draw, updateStatus.

The remaining obstacle to extracting it isn't the class, it's the 13 suites that slice @pure:edit-history and fabricate their own S — which the sliced command classes then share. Importing the real one would split that object in two. That rewrite is its own step, and suggest_position_wiring above is the pattern it will follow.

Verification

• npm run lint 0 errors, 10 warnings. node --test 88/88, pytest 248/248.
• Graph acyclic — keys → ui is new, and ui imports nothing.
• All five headless harnesses green. Codex preflight: NO ISSUES.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Suggested position marks now persist across save/load cycles, keeping previously marked notes marked after reload.
  • Enhanced roll-lock notice with clearer instructions for cycling same-pitch positions or switching to String view.

• Bug Fixes

  • Marked note suggestions are restored more reliably after edits and reloads.
  • Position-count/status updates stay synchronized with the currently active arrangement.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 9c2d0c36-1f9e-4cec-b1ad-f0a2e142c936

📥 Commits

Reviewing files that changed from the base of the PR and between 774a408 and 33f0c69.

📒 Files selected for processing (1)

• src/keys.js

---

📝 Walkthrough

Walkthrough

This PR relocates suggested-mark persistence and roll-lock-notice logic out of src/main.js into src/notes.js and src/keys.js, updates tests to use the real exported helpers and shared state, and adds a changelog entry for the migration step.

Changes

ES-module migration step 13

Layer / File(s)	Summary
Roll lock notice moved to src/keys.js src/keys.js, src/main.js	_rollLockNotice() now lives in src/keys.js with a setStatus import, and src/main.js keeps only the earlier registry reference while removing the standalone definition.
Suggested-mark persistence moved to src/notes.js src/notes.js, src/main.js	_suggestedCount, storage keying, JSON parsing, mark reattachment, save, and restore helpers are added in src/notes.js; the equivalent block is removed from src/main.js, which retains identifier references.
Test updates for real implementation wiring tests/suggest_position_persist.test.mjs, tests/suggest_position_wiring.test.mjs	The tests now import the live helpers from src/notes.js, share the real S state object, and stop extracting private helpers from src/main.js.
Changelog entry CHANGELOG.md	Adds a changelog note for the migration step, including the suggested-mark and roll-lock relocations and the reduced EditHistory coupling.

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

Sequence Diagram(s)

sequenceDiagram
  participant Editor
  participant Notes as src/notes.js
  participant Storage as localStorage

  Editor->>Notes: _saveSuggestedMarks()
  Notes->>Storage: write JSON marks by filename/arrIdx key
  Editor->>Notes: _restoreSuggestedMarks()
  Notes->>Storage: read JSON marks by filename/arrIdx key
  Notes->>Notes: _applySuggestedMarksPure(nn, marks, tol, addFn)
  Notes->>Editor: notes re-marked in WeakSet

Loading

Possibly related PRs

• got-feedBack/feedBack-plugin-editor#143: Introduces the suggested-position persistence and reattachment flow that this PR relocates into src/notes.js and wires through tests.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main refactor: moving suggested-mark persistence and the roll lock notice.
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 refactor/es-module-split-marks

---

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

[Copilot]

Pull request overview

This PR continues the editor ES-module migration (R2 step 13) by relocating suggested-mark persistence logic and the piano-roll “read-only” lock notice into the modules where their dependencies already live, further shrinking main.js and tightening seams ahead of extracting EditHistory.

Changes:

• Moved suggested-position mark counting + persistence helpers (_suggestedCount, _saveSuggestedMarks, _restoreSuggestedMarks, and related “pure” helpers) into src/notes.js alongside the WeakSet they operate on.
• Moved _rollLockNotice into src/keys.js next to _rollReadOnly, adding a setStatus import from src/ui.js.
• Updated tests to remove/limit sandboxing and ensure they drive the real shared S (and localStorage via globalThis.localStorage).

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/suggest_position_wiring.test.mjs	Adjusts the sandbox harness to share the real S and pass _suggestedCount as an import rather than slicing it.
tests/suggest_position_persist.test.mjs	Removes sandbox extraction; drives the real notes.js exports, real S, and a stubbed globalThis.localStorage.
src/notes.js	Adds suggested-mark count + persistence helpers next to _suggestedNotes and imports S from state.js.
src/main.js	Updates imports to consume moved suggested-mark helpers and _rollLockNotice from their new modules; deletes the in-file implementations/comments.
src/keys.js	Adds _rollLockNotice and imports setStatus from ui.js.
CHANGELOG.md	Documents step 13 moves and the related test harness changes.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[byrongamatos]

Fixed. The header claimed localStorage was the only browser surface in keys.js; _rollLockNotice reports through setStatus, which reaches document.getElementById. It now names both — that header is the thing a reader trusts when deciding whether the module is importable under node.