fix(nav): nobody may monkey-patch window.showScreen — add screen:changing, make the shell listen (#924)

[byrongamatos]

Closes #924. Stacked on #923.

window.showScreen was wrapped by three independent parties, each capturing whatever happened to be there at the time:

app.js publishes the raw function
  → static/v3/shell.js wrapped it (to call syncActive, and to map home → v3-songs)
  → the stems plugin wrapped it AGAIN (to tear down on leaving the player)

Plugins load asynchronously, so the chain linked up in whatever order the race settled. A capture taken before shell.js installed silently dropped the mapping it carried — and the library opened on the dead legacy #home screen. Testers saw that as "randomly, the library shows the old interface" (#923).

#923 fixed the symptom. This removes the cause: neither wrapper ever needed to be one.

Two events, and the distinction is the whole point

screen:changing	emitted before anything happens. "I am leaving from." Teardown/cancel here.
screen:changed	emitted after the DOM and data settle. "I am on id." Now also carries from.

screen:changing is new, and it exists because Codex caught me collapsing the two.

The stems plugin tore down its audio graph before showScreen did anything. screen:changed fires at the very end — after core awaits library and provider loads — so moving the plugin onto it would have delayed teardown behind a slow fetch, or skipped it entirely if that fetch threw, and stems would keep playing on a non-player screen.

A test pins the ordering: screen:changing must precede the first await.

shell.js is a plain screen:changed listener now — like app.js, audio-mixer.js and tour-engine.js already were. window.showScreen is an unwrapped function again, and tests/js/no_showscreen_monkeypatch.test.js fails CI if anything in static/ ever assigns to it again, so the hazard is structurally impossible rather than merely avoided.

And a fallback that could never fire

My retry-if-the-bus-is-late path listened for slopsmith:capabilities:ready. Core dispatches feedBack:capabilities:ready (capabilities.js:1536) — the slopsmith: name is the pre-DMCA event, and nothing has emitted it since the rename. Codex caught it.

A guard that cannot fire is worse than no guard: it reads as protection and is decoration.

_{The same dead-event bug turned out to be sitting in three of the stems plugin's fallbacks, where it has silently disabled its lifecycle wiring whenever the bus was late. Fixed in feedback-plugin-stems#38.}

Verification

A/B against origin/main:

nav highlight + topbar title follow identically with shell.js as a listener	✓
screen:changing → screen:changed fire in order, with the right {id, from}	✓
window.showScreen is unwrapped	✓
showScreen('home') still lands on v3-songs	✓

node 1053 · pytest 2425 · ESLint 0 · Codex 0.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Legacy “home” navigation now opens the Songs screen when the v3 interface is available.
  • Screen navigation events now include the previously active screen for improved transition tracking.
  • Navigation updates remain synchronized through screen-change events.

• Tests

  • Added coverage for legacy home routing, navigation event details, event timing, and prevention of conflicting navigation hooks.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5ca560f2-9e6f-4b7d-b839-cffc6c7c7fb2

📥 Commits

Reviewing files that changed from the base of the PR and between 545e569 and 0cb8b45.

📒 Files selected for processing (4)

• static/js/session.js
• static/v3/shell.js
• tests/js/no_showscreen_monkeypatch.test.js
• tests/js/show_screen_legacy_home.test.js

---

📝 Walkthrough

Walkthrough

showScreen now routes legacy home navigation to v3-songs when available and emits source-screen context. The v3 shell listens for screen:changed instead of wrapping window.showScreen, with tests covering routing, event ordering, payloads, and monkey-patch prevention.

Changes

Screen navigation

Layer / File(s)	Summary
Navigation routing and event payloads static/js/session.js, tests/js/show_screen_legacy_home.test.js	Legacy home requests map to v3-songs when present, while screen:changing and screen:changed include the previous screen ID. Tests verify routing conditions and ordering.
Shell event integration static/v3/shell.js, tests/js/no_showscreen_monkeypatch.test.js	The v3 shell synchronizes through screen:changed with capability-readiness retry behavior, and tests prevent window.showScreen monkey-patching while checking event payloads and ordering.

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

Sequence Diagram(s)

sequenceDiagram
  participant showScreen
  participant feedBack
  participant v3Shell
  showScreen->>feedBack: Emit screen:changing with id and from
  showScreen->>feedBack: Emit screen:changed with id and from
  feedBack->>v3Shell: Deliver screen:changed
  v3Shell->>v3Shell: Invoke syncActive(id)

Loading

🚥 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 clearly summarizes the navigation hook removal and event-based shell update.
Linked Issues check	✅ Passed	The PR implements the requested event-based navigation flow and removes the shell monkey-patch without changing unrelated scope.
Out of Scope Changes check	✅ Passed	The changes stay focused on navigation behavior, event payloads, and regression tests for the linked issue.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ 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/screen-change-event

---

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