refactor: migrate SpectatorListMenu to script setup

[Xinglan233]

Issue number

Refs #1412

Please check the following

• Do the tests still pass? (see Run the Tests)
• Is the code formatted properly? (see Linting (Formatting))
• For New Features (not applicable; this preserves existing behavior):
  • Have tests been added to cover any new features or fixes?
  • Has the documentation been updated accordingly?

Please describe additional details for testing this change

Converts SpectatorListMenu.vue to <script setup>, preserving the required Array prop, the translation binding, and the initially closed menu state. The template is unchanged, including its slots, selectors, translations, and responsive spectator count.

Validated before and after the migration with Node 22.21.0:

• npm run lint
• npm run test:unit (3 client and 83 Sails tests)
• npm run build
• Complete existing Cypress suite: 34 specs, 375 passing and 5 existing pending tests, no failures
• Live spectating Cypress spec (12 tests), covering spectator counts, an empty list, live joins/leaves, rejoining, and rematch

Temporary browser checks outside committed source also passed at 1440px and 390px, covering menu toggling and Escape, language switching, and live list updates.

Moving spectatingMenu to a setup ref changes $data from { spectatingMenu: false/true } to {}. The menu interactions passed the before/after checks above. In the current main branch, both the Rollbar import and plugin registration are commented out; if the plugin is re-enabled, its $data snapshot will no longer include this field when reporting a component error. The previous component had no explicit name, so no explicit name is lost.

Added a PropType<string[]> JSDoc annotation to the required runtime Array prop for the spectator usernames. An isolated frontend check with TypeScript 5.6.3 and vue-tsc 2.2.10 (allowJs and checkJs enabled) measures 420 → 419 diagnostics: the TS2322 in SpectatorListMenu.vue is removed, with no new diagnostics. The remaining 419 diagnostics are unchanged; no project typecheck tooling or configuration was added. This addresses one of the six components in #1412.

[itsalaidbacklife]

Thanks for this — the conversion itself is correct, and the write-up is unusually careful. Everything in it that I spot-checked holds up: Rollbar really is disabled on main (src/main.js:4 and :21, pending #1129), so the $data note is accurate and currently moot, and the component genuinely had no explicit name.

The migration is clean. BaseMenu is auto-registered by the <script setup> import, t and spectatingMenu are auto-exposed to the template, and the template is untouched. I checked for consumers that might depend on the old instance surface — GameView.vue:15 is the only usage and it binds a prop, with no template ref or $refs access anywhere, so dropping the implicit instance exposure is safe.

One thing to flag, and it is my fault rather than yours.

You were right not to claim a type-error reduction. I ran the check you couldn't, with vue-tsc against both commits:

origin/main a56bd1e6 : 420 errors total   SpectatorListMenu.vue: 1
this PR     508186be : 420 errors total   SpectatorListMenu.vue: 1

Byte-identical. The error survives the migration:

src/routes/game/components/SpectatorListMenu.vue(37,60):
  error TS2322: Type 'unknown' is not assignable to type 'PropertyKey'.

Line 37 is the template, not the script:

<v-list-item v-for="spectator in spectatingUsers" :key="spectator">

type: Array carries no element type, so spectator is unknown, and :key wants a PropertyKey. That has nothing to do with the Options API, which is why converting the script block does not move it.

#1412 told you these six files each had "one type error" that migration would clear. That was wrong — I picked those files by error count and size without checking what caused each error. Of the six, only FourDialog.vue has a migration-fixable error. The other five, including this one, are untyped props. I am correcting the issue now.

If you want this PR to close its stated goal, three lines do it:

defineProps({
  spectatingUsers: {
    /** @type {import('vue').PropType<string[]>} */
    type: Array,
    required: true,
  },
});

Verified: 420 → 419, this file to 0. string[] is the right type — src/stores/game.js:292 and :802 treat the entries as usernames.

Entirely your call. The refactor stands on its own as AGENTS.md compliance (<script setup> for components) whether or not you take the annotation, and I am happy to merge it either way.

Unrelated observation while I was in here: GameView.vue:15 passes :vuetify-display="$vuetify", a prop this component has never declared and does not use — it reads $vuetify.display.xs directly instead. It was a fallthrough attribute before your change and still is, so this PR changes nothing about it, but it looks vestigial and is probably worth deleting from the parent at some point.

[Xinglan233]

Thanks for this — the conversion itself is correct, and the write-up is unusually careful. Everything in it that I spot-checked holds up: Rollbar really is disabled on main (src/main.js:4 and :21, pending #1129), so the $data note is accurate and currently moot, and the component genuinely had no explicit name.

The migration is clean. BaseMenu is auto-registered by the <script setup> import, t and spectatingMenu are auto-exposed to the template, and the template is untouched. I checked for consumers that might depend on the old instance surface — GameView.vue:15 is the only usage and it binds a prop, with no template ref or $refs access anywhere, so dropping the implicit instance exposure is safe.

One thing to flag, and it is my fault rather than yours.

You were right not to claim a type-error reduction. I ran the check you couldn't, with vue-tsc against both commits:

origin/main a56bd1e6 : 420 errors total   SpectatorListMenu.vue: 1
this PR     508186be : 420 errors total   SpectatorListMenu.vue: 1

Byte-identical. The error survives the migration:

src/routes/game/components/SpectatorListMenu.vue(37,60):
  error TS2322: Type 'unknown' is not assignable to type 'PropertyKey'.

Line 37 is the template, not the script:

<v-list-item v-for="spectator in spectatingUsers" :key="spectator">

type: Array carries no element type, so spectator is unknown, and :key wants a PropertyKey. That has nothing to do with the Options API, which is why converting the script block does not move it.

#1412 told you these six files each had "one type error" that migration would clear. That was wrong — I picked those files by error count and size without checking what caused each error. Of the six, only FourDialog.vue has a migration-fixable error. The other five, including this one, are untyped props. I am correcting the issue now.

If you want this PR to close its stated goal, three lines do it:

defineProps({
  spectatingUsers: {
    /** @type {import('vue').PropType<string[]>} */
    type: Array,
    required: true,
  },
});

Verified: 420 → 419, this file to 0. string[] is the right type — src/stores/game.js:292 and :802 treat the entries as usernames.

Entirely your call. The refactor stands on its own as AGENTS.md compliance (<script setup> for components) whether or not you take the annotation, and I am happy to merge it either way.

Unrelated observation while I was in here: GameView.vue:15 passes :vuetify-display="$vuetify", a prop this component has never declared and does not use — it reads $vuetify.display.xs directly instead. It was a fallthrough attribute before your change and still is, so this PR changes nothing about it, but it looks vestigial and is probably worth deleting from the parent at some point.

Thanks for checking! Added the suggested PropType<string[]> annotation. The type check now reports 419 errors instead of 420, with no new diagnostics. All local checks, including the full Cypress suite, pass.

[itsalaidbacklife]

Requesting one change, and the direction has shifted since my earlier comment — apologies for moving the target.

What changed: the project is adopting TypeScript for frontend code rather than JSDoc type annotations (#1399). So instead of the /** @type {import('vue').PropType<string[]>} */ form I suggested above, please use lang="ts":

<script setup lang="ts">
import { ref } from 'vue';
import { useI18n } from 'vue-i18n';
import BaseMenu from '@/components/BaseMenu.vue';

defineProps<{ spectatingUsers: string[] }>();

const { t } = useI18n();
const spectatingMenu = ref(false);
</script>

That is the whole change — everything else in your PR stands as-is.

Verified, so you do not have to take it on faith:

origin/main                     420 errors    SpectatorListMenu.vue: 1
this PR as it stands            420 errors    SpectatorListMenu.vue: 1
with <script setup lang="ts">   419 errors    SpectatorListMenu.vue: 0
vite build                      exit 0

string[] is the right type: src/stores/game.js:292 and :802 treat the entries as usernames.

No new dependency or build config is needed. Vite already transpiles lang="ts" through esbuild — I built this exact file against the current node_modules and it succeeded. Type checking will come later from vue-tsc in CI (#1400), but nothing about this PR has to wait for that.

Why I am asking rather than just merging: four comparable migrations landed this week (#1416–#1419) and the frontend error count did not move at all — 420 before, 420 after. They were correct refactors, but each left its type error in place, so they now need a second pass (#1428). Catching yours here saves it from the same fate.

Thanks for the careful work on this one — the write-up on the $data / Rollbar interaction was genuinely useful, and the migration itself needed no changes.

[Xinglan233]

Requesting one change, and the direction has shifted since my earlier comment — apologies for moving the target.

What changed: the project is adopting TypeScript for frontend code rather than JSDoc type annotations (#1399). So instead of the /** @type {import('vue').PropType<string[]>} */ form I suggested above, please use lang="ts":

<script setup lang="ts">
import { ref } from 'vue';
import { useI18n } from 'vue-i18n';
import BaseMenu from '@/components/BaseMenu.vue';

defineProps<{ spectatingUsers: string[] }>();

const { t } = useI18n();
const spectatingMenu = ref(false);
</script>

That is the whole change — everything else in your PR stands as-is.

Verified, so you do not have to take it on faith:

origin/main                     420 errors    SpectatorListMenu.vue: 1
this PR as it stands            420 errors    SpectatorListMenu.vue: 1
with <script setup lang="ts">   419 errors    SpectatorListMenu.vue: 0
vite build                      exit 0

string[] is the right type: src/stores/game.js:292 and :802 treat the entries as usernames.

No new dependency or build config is needed. Vite already transpiles lang="ts" through esbuild — I built this exact file against the current node_modules and it succeeded. Type checking will come later from vue-tsc in CI (#1400), but nothing about this PR has to wait for that.

Why I am asking rather than just merging: four comparable migrations landed this week (#1416–#1419) and the frontend error count did not move at all — 420 before, 420 after. They were correct refactors, but each left its type error in place, so they now need a second pass (#1428). Catching yours here saves it from the same fate.

Thanks for the careful work on this one — the write-up on the $data / Rollbar interaction was genuinely useful, and the migration itself needed no changes.

Thanks for clarifying the TypeScript direction. I have prepared the exact change you suggested locally. The component builds successfully, but I found an ESLint prerequisite that needs to be addressed before pushing it. I reproduced this independently on both the current PR head and upstream main to check whether it was specific to my local setup.

1. What I changed
   The current PR already contains the script setup migration and the JSDoc prop annotation from your earlier suggestion. For this follow-up, I made only two changes:

• Changed <script setup> to <script setup lang="ts">.
• Replaced the runtime/JSDoc prop declaration with defineProps<{ spectatingUsers: string[] }>();.
  Against the current PR head, this changes only SpectatorListMenu.vue, with 2 lines added and 8 removed. The imports, useI18n binding, ref(false) menu state, and entire template remain unchanged. I have not added dependencies or changed the ESLint, Vite, or other project configuration.
  The requested TS follow-up has not been committed or pushed yet. The remote PR still contains the JavaScript/JSDoc version.

2. Independent reproduction
   I tested these two remote commits in separate clean checkouts:

• PR refactor: migrate SpectatorListMenu to script setup #1415: 487a175
• Upstream main: 71a0f2d
  For each checkout, I installed dependencies from its unchanged lockfile, retained the repository’s .npmrc, and ran the official npm run lint command from the repository root. I then applied the exact TS script from your review, ran lint again, restored the original component, and repeated lint once more.
  Both checkouts produced the same sequence:
• Unmodified baseline: exit 0.
• Exact TS replacement: exit 1, with one new parsing error.
• Original component restored: exit 0 again.
  The new error is:
  src/routes/game/components/SpectatorListMenu.vue
  50:38 Parsing error: Unexpected token ]
  Both baselines also have one existing vue/no-template-shadow warning in StatsLeaderboardCell.vue. That warning remains unchanged; the new failure is the parsing error in SpectatorListMenu.vue.
  Main still has the older Options API version of this component, so applying the review snippet there produces a different component-only diff. The resulting TS component is identical in both checkouts, and both encounter the same parser failure.

3. Environment and configuration checks
   The independent reproduction used:

• Node.js 22.21.0 and npm 10.9.4
• ESLint 9.38.0
• eslint-plugin-vue 10.5.1
• vue-eslint-parser 10.2.0
• Espree 10.4.0
• Vite 7.1.12 and @vitejs/plugin-vue 6.0.1
• @vue/compiler-sfc 3.5.22
  Node 22.21 matches the repository’s engines, .nvmrc, and CI setup. Each checkout had a freshly installed dependency tree. The installed package versions and integrity metadata matched its lockfile, and the repository’s legacy-peer-deps=true setting was retained.
  I also checked ESLint’s resolved configuration for this specific component. It selected the checkout’s root eslint.config.js, with vue-eslint-parser as the outer parser and no configured script parser in parserOptions. The PR and main ESLint configuration files are identical.
  Neither dependency tree contains @typescript-eslint/parser or typescript. The baseline and modified component were checked with the same resolved configuration and tool versions.

4. Why the build passes but lint fails
   Your observation about Vite is confirmed: npm run build succeeds with the exact TS component and the existing dependencies.
   ESLint reads the original Vue source separately from Vite’s build pipeline. It does not inherit Vite’s TypeScript transpilation support. The Vue parser handles the SFC structure, but without a TypeScript-capable script parser configured beneath it, the script is parsed as JavaScript through Espree.
   The typed defineProps declaration contains TypeScript syntax that this parser cannot handle. The Unexpected token ] error occurs during parsing, before ordinary lint rules can inspect the script.
   This distinction is covered by the Vue ESLint documentation:
   https://eslint.vuejs.org/user-guide/#how-to-use-a-custom-parser
5. Other validation
   On the TS candidate, the implementation pass completed:

• 86 unit tests.
• The production build.
• 12 spectator E2E tests.
• Two desktop/narrow-screen checks.
  The independent investigation separately repeated the build and all 86 unit tests successfully. It did not repeat the full Cypress suite, so I am not reporting a new full-suite pass for this TS follow-up.
  In the earlier fixed-toolchain type-check comparison, the existing JSDoc version and the TS version both produced 419 total diagnostics, with zero in SpectatorListMenu.vue. Their diagnostic logs were identical. This agrees with [DevEx]: Add TS prop types to nine already-migrated script setup components #1428’s clarification that JSDoc provides the equivalent type information while TypeScript is the preferred frontend direction. The separate lint investigation did not rerun that type-check measurement.

6. CI relevance
   The lint job in .github/workflows/pr-actions.yml calls the reusable run-npm-script.yml workflow with npm run lint. That workflow uses Node 22.21 and npm ci before running the supplied command.
   The repository’s lint script is eslint --ext .js,.vue ., which is also the command exercised through npm run lint in both clean local comparisons.
   I have not pushed this candidate or triggered a new GitHub Actions run, so this is a reproduced local failure of the official CI command, rather than a claim that a remote run for this unpublished change has failed.
7. Proposed solution and scope
   The standard prerequisite would be to add compatible versions of @typescript-eslint/parser and its typescript peer as devDependencies, then configure the TypeScript script parser under the existing vue-eslint-parser. The outer Vue parser should remain in place so that Vue templates continue to be parsed correctly.
   Because the repository uses legacy-peer-deps=true, I would make the TypeScript peer an explicit approved dependency rather than assume it will be installed automatically.
   A narrow configuration could apply only to SpectatorListMenu.vue. Shared support for Vue components may make more sense for the additional TS conversions in [DevEx]: Add TS prop types to nine already-migrated script setup components #1428, but I would leave that scope decision to you.
   Including the narrow prerequisite here would expand the change to four files:

• SpectatorListMenu.vue
• eslint.config.js
• package.json
• package-lock.json
  This parser setup would not itself require a Vite change, a type-aware lint preset, or a new tsconfig. Adding vue-tsc for [DevEx]: Add TypeScript tooling, ESLint TS parser, typecheck scripts, and CI jobs #1400 would also not automatically configure ESLint’s script parser.
  I have not installed these additional dependencies or implemented the configuration proposal. It would need a clean install and validation once approved; I do not want to present an untested configuration as a completed fix.
  Would you prefer to include the minimal parser setup in this PR, or land shared frontend TS lint support separately before applying this component’s TS follow-up? If it belongs here, should the configuration cover only this component or the frontend migrations planned in [DevEx]: Add TS prop types to nine already-migrated script setup components #1428?
  I have kept the follow-up local for now because it requires approval for new dependencies, and I wanted to check the intended scope before expanding this otherwise small component change.

[itsalaidbacklife]

Withdrawing my change request — please disregard it. This PR is good as it stands.

I asked you to convert to <script setup lang="ts">. That would have broken your build, and I should have checked before asking. npm run lint is a blocking CI job and this repo has no TypeScript-aware ESLint parser, so any TS syntax fails to parse:

SpectatorListMenu.vue
  50:38  error  Parsing error: Unexpected token ]

It is not specific to the array type — every form fails:

const x: string = 'a';            ->  Parsing error: Unexpected token :
defineProps<{ n: number }>();     ->  Parsing error: Unexpected token )
export const y: string = 'b';     ->  Parsing error: Unexpected token :   (plain .ts)

eslint.config.js uses vue-eslint-parser, which delegates <script> to espree unless a TS parser is configured, and @typescript-eslint/parser is not installed. So lang="ts" cannot land anywhere in this repo until that tooling is added — which is now tracked in #1400, including the verified fix ([email protected] + parser wiring, confirmed to parse all four cases above).

Nothing for you to change. Your migration is correct: BaseMenu auto-registers via the <script setup> import, t and spectatingMenu are auto-exposed, the template is untouched, and GameView.vue:15 is the only consumer with no $refs access. The $data / Rollbar note was accurate too.

The one type error in this file (TS2322 on :key="spectator", from the untyped Array prop) stays open and is tracked in #1428 along with eight other already-migrated components in the same position. It needs defineProps<{ spectatingUsers: string[] }>(), which can only be written once #1400 lands.

Sorry for the churn — you got caught between a direction change and a tooling gap, neither of which was yours. Thanks for the patience, and for a genuinely careful PR.