Skip to content

refactor(v2): extract a shared player shell - #4301

Merged
gantoine merged 3 commits into
masterfrom
refactor/v2-player-shell
Aug 29, 2026
Merged

refactor(v2): extract a shared player shell#4301
gantoine merged 3 commits into
masterfrom
refactor/v2-player-shell

Conversation

@gantoine

@gantoine gantoine commented Aug 29, 2026

Copy link
Copy Markdown
Member

Description
Explain the changes or enhancements you are proposing with this pull request.

The v2 player views duplicate their chrome. EmulatorJS.vue, Ruffle.vue and JsDos.vue each carry their own copy of the block every player opens with (synchronous hero seeding, title, platform label, background-art watch), and Ruffle.vue and JsDos.vue additionally hand-roll the cover column, settings card, back buttons and loading spinner that any simple player needs.

This extracts three seams:

  • usePlayerHero (src/v2/composables/usePlayerHero/) — the seed / hero / title / platform-label block plus the background-art watch. Adopted by all three players. The caller keeps ownership of the rom ref, because EmulatorJS needs a deep one for the save and state lists it mutates.
  • PlayerShell (src/v2/components/Player/PlayerShell.vue) — the cover column, settings card, play and back buttons, the full-bleed running stage and the loading state, with settings, stage and brand slots. Ruffle keeps only its colour picker, brand strip and stage; js-dos keeps only its fullscreen switch, browser-save note and stage.
  • usePlayerNav (src/v2/composables/usePlayerNav/) — the two back links, shared by the shell and by EmulatorJS. EmulatorJS's copy navigated off rom.value?.id, so it pushed undefined params during exactly the seed window the hero seeding exists to cover; it now uses the route id like the shell does.

EmulatorJS.vue deliberately opts out of PlayerShell: its hero sits inside a card with an alt-art glow and it lays out three panels on its own breakpoints. It takes the composables alone, and while it was open it also drops its hand-rolled @keyframes spinner for RSpinner.

Stream.vue is left alone: its label comes from the streaming container, its background art clears while playing, and it has its own back route.

Net -638 / +558 across nine files, and the duplication is gone rather than moved.

Why now: this fell out of a cleanup pass on #4061 (js-dos player), which landed as a third near-verbatim copy of the same chrome — JsDos.vue and Ruffle.vue shared ~245 lines. Now that #4061 is on master, this PR is rebased on it and converts JsDos.vue too, so PlayerShell gains its second consumer here.

Behaviour: no intended change. The one visible difference is that Ruffle's hand-rolled loading spinner is now RSpinner, matching the other players (same 40px / 2px / brand-coloured arc, and it drops a bespoke @keyframes). The js-dos view keeps its own quit/save flow, route-leave guard and beforeunload warning; only its chrome moves into the shell.

Checklist
Please check all that apply.

  • I've tested the changes locally
  • I've updated relevant comments
  • I've assigned reviewers for this PR
  • I've added unit tests that cover the changes

Verified: npm run typecheck, npm run test (838 tests / 74 files), npm run build and trunk fmt && trunk check all pass. PlayerShell.test.ts is new and covers the play/quit emits, the missing-platform guard and the loading branch; JsDos.test.ts's 15 existing cases move their selectors onto the shell's classes. Not yet exercised in a browser, and since the change moves scoped CSS into a child component that is where a regression would hide: the Ruffle and js-dos pre-game panels and running stages want a look in both themes before merge.

Screenshots (if applicable)

n/a


AI assistance: this PR was written with Claude Code (Opus 5). The duplication was identified by an automated review pass over #4061; the extraction, the decision on which views adopt which seam, and the verification above were carried out by the agent under my direction and reviewed by me.

Copilot AI lite review requested due to automatic review settings August 29, 2026 14:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@greptile-apps

greptile-apps Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR extracts duplicated v2 player presentation and hero-state logic without changing the player lifecycle.

  • Adds PlayerShell for shared cover, controls, loading, navigation, and running-stage chrome.
  • Adds usePlayerHero for synchronous ROM seeding, title and platform labels, page titles, and background artwork.
  • Migrates Ruffle to both abstractions and EmulatorJS to the shared composable only.

Confidence Score: 5/5

The PR appears safe to merge, with no concrete changed-code-triggered failure identified.

The extraction preserves the existing ROM fetch, hero seeding, playback lifecycle, navigation, scoped slot styling, and player-specific behavior across both consumers.

Reviews (1): Last reviewed commit: "refactor(v2): extract a shared player sh..." | Re-trigger Greptile

greptile-apps[bot]
greptile-apps Bot previously approved these changes Aug 29, 2026
gantoine and others added 2 commits August 29, 2026 15:44
EmulatorJS.vue and Ruffle.vue each carry their own copy of the block every v2
player opens with: the synchronous hero seeding, title, platform label and
background-art watch. Ruffle additionally hand-rolls the cover column,
settings card, back buttons and loading spinner that any simple player needs.

Extract two seams:

  * usePlayerHero — the seed/hero/title/platform-label block plus the
    background-art watch, adopted by both players. The caller keeps ownership
    of the `rom` ref because EmulatorJS needs a deep one for the save and
    state lists it mutates.

  * PlayerShell — the cover column, settings card, play and back buttons, the
    full-bleed running stage and the loading state, with `settings`, `stage`
    and `brand` slots. Ruffle keeps only its colour picker, brand strip and
    stage. EmulatorJS deliberately opts out: its hero sits inside a card with
    an alt-art glow and it lays out three panels on its own breakpoints, so it
    takes the composable alone.

Stream.vue is left alone: its label comes from the streaming container, its
background art clears while playing, and it has its own back route.

No intended behaviour change. Ruffle's hand-rolled loading spinner is now
RSpinner, matching the other players.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The js-dos player landed on master with its own copy of the pre-game
chrome. It now takes `usePlayerHero` and `PlayerShell` and keeps only the
fullscreen switch, the browser-save note and its stage.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gantoine
gantoine force-pushed the refactor/v2-player-shell branch from 2aa9ee6 to 68168f8 Compare August 29, 2026 19:48
@greptile-apps
greptile-apps Bot dismissed their stale review August 29, 2026 19:48

Dismissed because a newer commit was pushed; Greptile will re-review the current head.

Follow-up pass over the extraction:

- `usePlayerNav` gives the two back links one home. EmulatorJS's copy read
  `rom.value?.id`, so it pushed undefined params during the seed window the
  hero seeding exists to cover.
- EmulatorJS drops its hand-rolled spinner for `RSpinner` and takes
  `heroRom` for its render guard, so `heroSeed` leaves the composable's
  public surface.
- The shell owns the full-width brand row instead of letting a consumer
  reach across the seam for `grid-column`.
- Ruffle's rom ref is shallow, matching the other simple players.
- `PlayerShell.test.ts` covers the play/quit emits, the missing-platform
  guard and the loading branch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gantoine
gantoine merged commit 78777e6 into master Aug 29, 2026
12 checks passed
@gantoine
gantoine deleted the refactor/v2-player-shell branch August 29, 2026 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants