feat(editor): onboarding signposts + first-win cues (workspace-shell C2) - #222
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change adds persistent onboarding signposts and one-time first-win cues, integrates them with editor actions and session lifecycle, and adds animated UI surfaces, reduced-motion handling, and focused tests. ChangesOnboarding cues
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant EditorAction
participant signposts.js
participant localStorage
participant EditorDOM
EditorAction->>signposts.js: Record navigation, Tempo Map, or lock action
signposts.js->>localStorage: Read and persist seen state
signposts.js->>EditorDOM: Show signpost or first-win cue
EditorDOM-->>signposts.js: Dismiss signpost
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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.
Inline comments:
In `@CHANGELOG.md`:
- Around line 25-36: Update the signpost and first-win cue state handling
associated with _lsSet so storage failures retain seen flags in an in-memory
session fallback, preserving one-shot behavior during the current session and
across reloads when localStorage is available. Alternatively, revise this
changelog entry to explicitly describe persistence as best-effort rather than
permanent.
In `@screen.html`:
- Around line 209-216: Update the editor-signpost container to include
role="status" and aria-live="polite", preserving its existing classes, content,
and dismissal behavior so dynamically revealed onboarding text is announced.
In `@src/draw.js`:
- Line 56: Update the arrangement-switch handling around notes() and
_maybeFireFirstCovered() so changing S.currentArr cannot trigger a false
empty-to-covered cue without an authored edit. Re-seed the coverage baseline for
each arrangement or skip the cue when only the view context changes, while
preserving cues caused by actual coverage changes.
In `@src/signposts.js`:
- Around line 73-81: In `_showSignpost`, `_fireCueOnce`, and `_showCue`, perform
the relevant `#editor-signpost` or `#editor-cue` element lookup and return if
absent before writing the localStorage “seen” flag. Persist the one-shot flag
only after the surface element is confirmed present, while preserving the
existing rendering behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d4e4c78-e580-4ff9-b0a4-debd3711c383
📒 Files selected for processing (10)
CHANGELOG.mdassets/v3-theme.cssscreen.htmlsrc/draw.jssrc/file-ops.jssrc/input.jssrc/main.jssrc/signposts.jssrc/tempo.jstests/signposts.test.mjs
Charrette §3.2/§3.4. Two quiet, un-gamified onboarding surfaces, both editor-
pref (localStorage, never the pack), in a new src/signposts.js.
SIGNPOSTS — suggest-only, action-triggered, one-shot, permanently dismissible,
capped at ≤3. They only point at the menu/shortcut in words; they never move or
add a surface. Two hand-audited entries:
- grid-fit: repeated resnaps (the canonical "fighting the grid" signal) →
"the beat grid may be off — line it up with the Tempo tools (T), or Ctrl+K";
suppressed once Tempo Map has been opened.
- sections: many navigation jumps on a long, section-less song → "mark
sections with Shift+M".
Wired at the resnap / beat+note+bookmark-jump / Tempo-Map-entry action sites;
never a timer or idle nag. A shown signpost is marked seen and never reappears.
FIRST-WIN CUES — calm, one-time visual acknowledgements of a CORRECTNESS
milestone (no sound/score/token): the first barline locked to the recording,
and the first section to gain content. The covered cue baselines against the
loaded chart (reset on song load) so opening an already-charted song stays
quiet — it fires only on an empty→covered edit transition.
Completeness shading is unchanged (_sectionCoveragePure already presence-only,
never a density target); a test pins that an empty-by-intent span is never
flagged.
tests/signposts.test.mjs (7 cases): eligibility, one-shot + dismiss, sections
gating, cue-once, covered-cue load-vs-edit baseline, shading presence-only.
Verified live: 3 resnaps surface the grid-fit signpost with the right copy;
dismiss hides it and it never returns; no console errors. npm test 115 green,
lint 0 errors.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
ChrisBeWithYou
force-pushed
the
feat/editor-signposts
branch
from
July 12, 2026 20:09
be9c500 to
64f6ca4
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/signposts.test.mjs (1)
52-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing regression tests for missing DOM elements and broken
localStorage.The PR's commit summary states regression tests were added for missing elements and broken
localStorage, but the harness here (Lines 13-29) always provides working storage and present elements — no test exercisesdocument.getElementByIdreturningnullor a throwinglocalStorage. This gap is directly why the flag-before-element-check regression insrc/signposts.js(Lines 73-81, 104-121) would go unnoticed by this suite.Consider adding cases such as:
t('a missing `#editor-signpost` never burns the one-shot flag', () => { reset(); const saved = _els['editor-signpost']; _els['editor-signpost'] = null; Object.assign(S, { sections: [], duration: 0 }); for (let i = 0; i < 3; i++) _signpostNote('gridFight'); assert.ok(!_signpostSeen('grid-fit'), 'flag must not be set when the surface never rendered'); _els['editor-signpost'] = saved; });🤖 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/signposts.test.mjs` around lines 52 - 121, Add regression tests in the signpost test suite for unavailable UI and storage: make `#editor-signpost` resolve to null, verify triggering grid-fit does not mark _signpostSeen or burn the one-shot state, then restore the element; also replace or stub localStorage with a throwing implementation and verify signpost/cue behavior remains safe without crashing. Use reset, _signpostNote, _signpostSeen, and the existing _els/S harness symbols.
🤖 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.
Inline comments:
In `@src/main.js`:
- Line 120: Update the onboarding cue elements `#editor-signpost` and `#editor-cue`
to include role="status" and aria-live="polite", ensuring both message
containers expose live-region semantics while preserving their existing markup
and behavior.
In `@src/signposts.js`:
- Around line 73-81: Update _showSignpost to look up and validate
`#editor-signpost` before calling _lsSet, and return a success indicator only
after the signpost is rendered. Update _showCue similarly to report whether
rendering succeeded, then change _fireCueOnce to persist its one-shot flag only
when _showCue reports success; preserve retry eligibility when either target
element is absent.
- Around line 23-24: Update the storage helpers _lsGet and _lsSet to use a
module-level in-memory Map when localStorage access throws, preserving reads and
writes for the session. Keep localStorage as the primary store and ensure
_signpostSeen and _cueSeen retain their seen state when persistent storage is
unavailable.
---
Nitpick comments:
In `@tests/signposts.test.mjs`:
- Around line 52-121: Add regression tests in the signpost test suite for
unavailable UI and storage: make `#editor-signpost` resolve to null, verify
triggering grid-fit does not mark _signpostSeen or burn the one-shot state, then
restore the element; also replace or stub localStorage with a throwing
implementation and verify signpost/cue behavior remains safe without crashing.
Use reset, _signpostNote, _signpostSeen, and the existing _els/S harness
symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8134d065-52e6-4944-96e7-83a7011effa5
📒 Files selected for processing (10)
CHANGELOG.mdassets/v3-theme.cssscreen.htmlsrc/draw.jssrc/file-ops.jssrc/input.jssrc/main.jssrc/signposts.jssrc/tempo.jstests/signposts.test.mjs
🚧 Files skipped from review as they are similar to previous changes (5)
- src/draw.js
- screen.html
- CHANGELOG.md
- src/file-ops.js
- assets/v3-theme.css
| import { _fretboardStripRefresh, editorToggleFretboardStrip, initFretboardStrip } from './fretboard-strip.js'; | ||
| import { initMenuBar } from './menu-bar.js'; | ||
| import { initToolbars } from './toolbars.js'; | ||
| import { editorDismissSignpost } from './signposts.js'; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm role/aria-live wiring on the onboarding surfaces in screen.html
rg -n -B2 -A2 'id="editor-(signpost|cue)"' screen.htmlRepository: got-feedBack/feedBack-plugin-editor
Length of output: 1304
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the onboarding markup with surrounding context
sed -n '200,228p' screen.html | cat -nRepository: got-feedBack/feedBack-plugin-editor
Length of output: 4115
Add live-region semantics to the onboarding cues. #editor-signpost and #editor-cue still render as plain <div>s, so their messages won’t be announced; add role="status" and aria-live="polite" to both.
🤖 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 `@src/main.js` at line 120, Update the onboarding cue elements `#editor-signpost`
and `#editor-cue` to include role="status" and aria-live="polite", ensuring both
message containers expose live-region semantics while preserving their existing
markup and behavior.
# Conflicts: # CHANGELOG.md # src/input.js
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@src/signposts.js`:
- Around line 15-30: Update _lsSet to mirror every successful write into
_memStore, and update _lsGet to consult that cache when localStorage returns
null or an empty value. Preserve the existing localStorage-first behavior and
fallback handling so one-shot flags remain available when persistent storage is
unavailable or full.
In `@src/tempo.js`:
- Around line 882-895: Clear S.tempoSelMulti before initializing the tempo-beat
drag in the _tempoSubBeatAtX handling block, alongside resetting S.tempoSel and
_tapTempo, so stale multi-selection state cannot affect subsequent deletion.
In `@tests/signposts.test.mjs`:
- Around line 134-151: Update the test setup around reset() to also reset or
reload the signpost module’s in-memory fallback maps used by _signpostSeen and
_fireCueOnce. Ensure this is test-only and clears fallback state between tests
without removing production session persistence, so IDs such as “grid-fit” and
“mem-cue” cannot leak across test cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 11f6dd79-045b-41ab-9751-e70d16eab51f
📒 Files selected for processing (8)
CHANGELOG.mdscreen.htmlsrc/file-ops.jssrc/input.jssrc/main.jssrc/signposts.jssrc/tempo.jstests/signposts.test.mjs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/file-ops.js
- CHANGELOG.md
- screen.html
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@src/signposts.js`:
- Around line 15-30: Update _lsSet to mirror every successful write into
_memStore, and update _lsGet to consult that cache when localStorage returns
null or an empty value. Preserve the existing localStorage-first behavior and
fallback handling so one-shot flags remain available when persistent storage is
unavailable or full.
In `@src/tempo.js`:
- Around line 882-895: Clear S.tempoSelMulti before initializing the tempo-beat
drag in the _tempoSubBeatAtX handling block, alongside resetting S.tempoSel and
_tapTempo, so stale multi-selection state cannot affect subsequent deletion.
In `@tests/signposts.test.mjs`:
- Around line 134-151: Update the test setup around reset() to also reset or
reload the signpost module’s in-memory fallback maps used by _signpostSeen and
_fireCueOnce. Ensure this is test-only and clears fallback state between tests
without removing production session persistence, so IDs such as “grid-fit” and
“mem-cue” cannot leak across test cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 11f6dd79-045b-41ab-9751-e70d16eab51f
📒 Files selected for processing (8)
CHANGELOG.mdscreen.htmlsrc/file-ops.jssrc/input.jssrc/main.jssrc/signposts.jssrc/tempo.jstests/signposts.test.mjs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/file-ops.js
- CHANGELOG.md
- screen.html
🛑 Comments failed to post (3)
src/signposts.js (1)
15-30: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Mirror fallback writes in the read path.
If
localStorage.setItem()throws because storage is full,getItem()can still returnnull;_lsGet()then ignores_memStore, so one-shot flags repeat on every qualifying action. Cache successful writes too and consult the memory cache when the persistent read is empty.🛠️ Proposed fix
function _lsGet(k) { - try { return localStorage.getItem(k); } - catch (_) { return _memStore.has(k) ? _memStore.get(k) : null; } + try { + const value = localStorage.getItem(k); + return value ?? (_memStore.has(k) ? _memStore.get(k) : null); + } catch (_) { + return _memStore.has(k) ? _memStore.get(k) : null; + } } function _lsSet(k, v) { - try { localStorage.setItem(k, v); } - catch (_) { _memStore.set(k, String(v)); } + const value = String(v); + _memStore.set(k, value); + try { localStorage.setItem(k, value); } catch (_) { /* private mode */ } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// All one-shot state is a single localStorage flag per id when storage is // available, with an in-memory fallback for private/sandboxed sessions. import { S } from './state.js'; const LS_SIGNPOST = (id) => `editorSignpost:${id}`; const LS_CUE = (id) => `editorCue:${id}`; const _memStore = new Map(); function _lsGet(k) { try { const value = localStorage.getItem(k); return value ?? (_memStore.has(k) ? _memStore.get(k) : null); } catch (_) { return _memStore.has(k) ? _memStore.get(k) : null; } } function _lsSet(k, v) { const value = String(v); _memStore.set(k, value); try { localStorage.setItem(k, value); } catch (_) { /* private mode */ }🤖 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 `@src/signposts.js` around lines 15 - 30, Update _lsSet to mirror every successful write into _memStore, and update _lsGet to consult that cache when localStorage returns null or an empty value. Preserve the existing localStorage-first behavior and fallback handling so one-shot flags remain available when persistent storage is unavailable or full.src/tempo.js (1)
882-895: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n -A35 -B10 'tempo-beat|_tempoMapOnMouseUp|tempoSelMulti' src/tempo.js src/input.jsRepository: got-feedBack/feedBack-plugin-editor
Length of output: 47253
Clear
S.tempoSelMultibefore starting a sub-beat drag
src/tempo.js:882-895—Deleteprefers the multi-selection, so an old range can still be removed after atempo-beatdrag. ClearS.tempoSelMultihere too.🤖 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 `@src/tempo.js` around lines 882 - 895, Clear S.tempoSelMulti before initializing the tempo-beat drag in the _tempoSubBeatAtX handling block, alongside resetting S.tempoSel and _tapTempo, so stale multi-selection state cannot affect subsequent deletion.tests/signposts.test.mjs (1)
134-151: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reset the fallback store between tests.
reset()clears only_store; it does not clear the signpost module’s in-memory fallback populated whenlocalStoragethrows. A later test using the same IDs—or this test run in a different order—can inherit_signpostSeen/_cueSeenstate from this case, making the suite order-dependent. Add a test-only module reset/reload for the fallback map without clearing production session persistence.🤖 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/signposts.test.mjs` around lines 134 - 151, Update the test setup around reset() to also reset or reload the signpost module’s in-memory fallback maps used by _signpostSeen and _fireCueOnce. Ensure this is test-only and clears fallback state between tests without removing production session persistence, so IDs such as “grid-fit” and “mem-cue” cannot leak across test cases.
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
2 participants
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.
Workspace-shell Phase C2 (charrette §3.2/§3.4). Two quiet, un-gamified onboarding surfaces in a new
src/signposts.js, both editor-pref (localStorage, never the pack). Requires B5/C1 — both merged.Signposts (suggest-only)
Action-triggered, one-shot, permanently dismissible, capped at ≤3. They only point at the menu/shortcut in words — they never move or add a surface, and are never timer/idle-triggered. Two hand-audited entries:
A shown signpost is marked seen and never reappears.
First-win cues (calm, one-time)
Visual acknowledgements of a correctness milestone — no sound, %, score, or token (charrette §3.4: reward the music getting more accurate, not the user doing more):
Completeness shading
Unchanged —
_sectionCoveragePureis already presence-only ("ambient progress — never a score"). A test pins that an empty-by-intent span is never flagged (noincomplete/requiredconcept exists in the model).Tests & verification
tests/signposts.test.mjs(7 cases): eligibility predicate, one-shot + dismiss persistence, sections gating (jumps × no-sections × long song), cue-fires-once, covered-cue load-vs-edit baseline, and shading presence-only.npm test115 green,npm run lint0 errors (3 pre-existing warnings = main's ratchet).screen_markupstays green (new overlays are balanced siblings of the canvas).Independent lane from the tempo track. Next in this track: C3 — the two entry tours.
🤖 Generated with Claude Code
Summary by CodeRabbit