refactor(ui)!: remove the classic v2 shell — v3 is the only UI (R3a) - #871
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe v3 shell is now served from Changesv3 UI default
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Deletes `static/index.html`, the `/v2` route, and the `FEEDBACK_UI` v2/legacy
opt-out. `/` and `/v3` both serve `static/v3/index.html`, which has been the
default since 0.3.0.
This is step 0 of the core-frontend ES-module migration (R3a). Both shells load
the same `static/app.js`, so every later step of that migration — exposing the
window contract, the `defer` ordering fix, the `type="module"` flips — would
otherwise have to be made and verified twice. Removing the fallback now halves
that surface before any of it is touched.
Incidentally fixes a latent bug in `index()`: its guard read
`if getenv_compat("FEEDBACK_UI") or getenv_compat("FEEDBACK_UI") in ("v2", "legacy")`,
whose left operand is truthy for *any* non-empty value — so `FEEDBACK_UI=v3`
actually served the **v2** shell.
- `static/tailwind.min.css` regenerated: the content globs scanned the deleted
file, so v2-only utility classes are now purged (CI's tailwind-fresh job
rebuilds and diffs it).
- Constitution amended to 1.3.0 — Principle II's frontend file list now names
`static/v3/index.html`.
- Tests: 4 suites read the v2 shell (3 via a constructed `path.join` that a
literal grep misses). Their v2 halves are paired duplicates of v3 tests that
stay, so they are dropped; `alpha_warning_banner` and the capability-registry
script-order test retarget to `static/v3/index.html`.
BREAKING CHANGE: `FEEDBACK_UI=v2` / `=legacy` and the `/v2` route are gone.
Unset the variable and use `/`. No chart, settings, or plugin data changes, and
no plugin API changes — v3 reuses the same engine.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
force-pushed
the
feat/r3-v2-removal
branch
from
July 11, 2026 14:24
ef2cead to
f935971
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@CLAUDE.md`:
- Around line 128-134: Align the plugin documentation with the v3-only route
contract: in CLAUDE.md, update the checklist to verify both / and /v3 as v3
entrypoints; in docs/plugin-v3-ui.md, remove the obsolete wording that labels /
as v2. No code changes are required.
🪄 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: 88f43574-5421-4189-bae1-c349e915ed8e
📒 Files selected for processing (18)
.specify/memory/constitution.mdCHANGELOG.mdCLAUDE.mddocs/plugin-styles.mddocs/plugin-v3-ui.mdserver.pystatic/app.jsstatic/highway.jsstatic/index.htmlstatic/tailwind.min.cssstatic/v3/index.htmltailwind.config.jstests/js/alpha_warning_banner.test.jstests/js/song_restart.test.jstests/js/tuning_display.test.jstests/js/tuning_targets.test.jstests/test_env_compat.pytests/test_plugin_runtime_idempotence.py
💤 Files with no reviewable changes (4)
- tests/js/tuning_targets.test.js
- tests/js/song_restart.test.js
- static/index.html
- tests/js/tuning_display.test.js
The v3-only rewrite updated the intro paragraphs but left three lines that still instructed plugin authors to verify in 'both / (v2) and /v3' — now the same shell. Historical 'in v2 it was X' contrasts are kept: they still orient authors whose plugins also ship to users on older cores. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 11, 2026
byrongamatos
added a commit
that referenced
this pull request
Jul 11, 2026
One attribute. #871/#872/#874/#875 exist to make this line safe. app.js's 385 top-level `function` declarations stop being implicit `window` properties: 87 stay reachable via the explicit contract (#874's Object.assign block + the 47 pre-existing `window.X = X` assignments), and 298 become module-private. Verified NO unexposed name is read from outside app.js. Strict mode (modules are always strict) checked ahead of the flip: app.js parses clean as `sourceType: module` (no octal, dup params, `with`), and has no implicit globals, no `eval`/`new Function`, no top-level `this`. `registerShortcut` is called bare at 15 top-level sites but is assigned at `window.registerShortcut` (app.js:10387) before its first call (10648), and a bare identifier in a module still resolves through the global object — verified `typeof window.registerShortcut === 'function'` in the browser. HARD GATE — app.js IS the plugin loader: - /api/plugins script_type passthrough: editor/stems/studio = "module" - /api/plugins/stems/src/main.js -> 200; conditional GET -> 304 (live-edit ETag) - deep graph: stems/src/transport.js, editor/src/state.js -> 200 - window.loadPlugins present; 5 plugin screens mount; the 3 migrated plugins injected as <script type="module"> - 37 capability participants, 14 compatibility shims, bus + capabilities v1 Every one of the shell's 336 inline handlers resolves on window under module scope, and the A-Z rail / pagination execute 6/6 with no ReferenceError. A/B against origin/main: the ONLY unresolvable handler is `editorToggleStemMixer`, which is equally broken on main (a dead handler in the editor plugin — not defined anywhere in its source; pre-existing, flagged separately). Codex preflight raised a [P1] claiming restartCurrentSong / requestExitSong / editRegionInEditor / returnToEditorFromHighway would ReferenceError — FALSE POSITIVE. It scanned only #874's new Object.assign block and missed app.js's 47 scattered `window.X = X` assignments; all four are at app.js:7086/7204/8492/8511 and all four resolve as `function` in the browser with app.js loaded as a module. pytest 2396, node 1032/1032, ESLint 0 errors, tailwind-fresh clean. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Step 0 of the core-frontend ES-module migration (R3a). Deletes the legacy v2 shell so the rest of the migration only has to be made — and verified — once.
Why this goes first
/has servedstatic/v3/index.htmlsince 0.3.0;static/index.htmlis the legacy shell behind/v2/FEEDBACK_UI=v2. Both shells load the samestatic/app.js. So every later R3a step — exposing app.js'swindow.*contract, thedeferordering fix, thetype="module"flips — would otherwise land in two HTML files and be booted twice. Removing the fallback now halves that surface before any of it is touched.What's in it
static/index.html;index()serves v3 unconditionally;/v2route and theFEEDBACK_UIv2/legacy branch removed./v3stays as a back-compat alias.if getenv_compat("FEEDBACK_UI") or getenv_compat("FEEDBACK_UI") in ("v2", "legacy")— the left operand is truthy for any non-empty value, soFEEDBACK_UI=v3actually served the v2 shell.static/tailwind.min.cssregenerated. The content globs (./static/**/*.{html,js}) scanned the deleted file, so v2-only utility classes are purged — without this thetailwind-freshCI job (which rebuilds andgit diff --quiets the committed CSS) goes red.static/v3/index.html. Governance requires the amendment to ride with the code change, plus the CHANGELOG migration note and the CLAUDE.md update — all here.plugin-v3-ui.md,plugin-styles.md,CLAUDE.md, the v3 shell's own header comment) — they still described v3 as opt-in.Tests
4 suites read the v2 shell, not 1 — three build the path via
path.join(__dirname,'..','..','static','index.html'), which a literal grep forstatic/index.htmlnever sees. Worth knowing for the next person auditing consumers.tests/test_plugin_runtime_idempotence.pydefer/modulesteps hinge on, so it's kept and retargeted, not dropped)tests/js/alpha_warning_banner.test.js#library-section+ banner markup)tests/js/song_restart.test.js,tuning_display.test.js,tuning_targets.test.jstests/test_env_compat.pyFEEDBACK_UIonly as an arbitrary subject for theSLOPSMITH_*→FEEDBACK_*shim; retargeted toFEEDBACK_DEMO_MODE, assertions unchangedVerification
pytest2395 passed, 4 skipped.npm run test:js1028/1028.max-lineswarnings, unchanged)./→ 200 (v3 markers present),/v3→ 200,/v2→ 404.feedBack.on/emit),capabilities.version === 1,showScreen/playSong/loadPluginsall present, library renders. Zero real console errors — the only 4xx are three/api/song/<id>/art404s, which are the handler's own{"error":"no art"}for seeded packs that genuinely ship no cover art (pre-existing, unrelated).script_type=module+min_host,/api/plugins/stems/src/main.js→ 200, conditional GET → 304, deepersrc/transport.js→ 200.Migration
FEEDBACK_UI=v2/=legacyand/v2are gone — unset the variable and use/. No chart, settings, or plugin data changes and no plugin API changes; v3 reuses the same engine (app.js,highway.js,playSong,showScreen, the capability registry).🤖 Generated with Claude Code
Summary by CodeRabbit
/and/v3serve the v3 interface./v2route, and legacy UI-selection options.