Conversation
…sts; include health check for TCP port 8001
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Adds CI linting/typecheck and a Docker smoke test, while tightening TypeScript typings around uWebSockets usage in the backend.
Changes:
- Add GitHub Actions workflow to lint/type-check the backend and run a Docker build + healthcheck-based smoke test.
- Introduce ESLint flat config for the backend and update backend lint scripts/deps.
- Improve backend websocket typing by using
uWebSockets.jsgenerics andws.getUserData()instead ofany/ws.player.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds/updates ESLint/TS-ESLint/jiti/minimatch-related dependency resolutions. |
| back/src/scripts/footballScript.ts | Comments out an unused targeted chat helper. |
| back/src/scripts/defaultScript.ts | Minor TS cleanup in callbacks and variable initialization. |
| back/src/ecs/system/network/WebsocketSystem.ts | Types websocket handlers and stores player in getUserData(); adjusts message parsing and connection handling. |
| back/src/ecs/system/network/NetworkSystem.ts | Tightens broadcast payload type to Uint8Array. |
| back/src/ecs/entity/Player.ts | Types websocket parameter as WebSocket<unknown>. |
| back/src/ecs/component/WebsocketComponent.ts | Types stored websocket as WebSocket<unknown>. |
| back/package.json | Updates lint scripts, adds ESLint flat-config deps, and adds overrides. |
| back/eslint.config.ts | Adds new ESLint flat config file for the backend. |
| Dockerfile | Adds a healthcheck probing TCP port 8001. |
| .github/workflows/ci.yml | New CI workflow for lint/typecheck + Docker smoke test. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (await this.isRateLimited(ip)) { | ||
| // Respond to the client indicating that the connection is rate limited | ||
| return ws.close(429, 'Rate limit exceeded') | ||
| return ws.close() |
There was a problem hiding this comment.
onConnect still says it will respond to the client when rate-limited, but now it just calls ws.close() with no close code/reason. If you want clients to handle this, send a meaningful WebSocket close code/reason (valid WS codes are 1000–4999) or reject the upgrade with an HTTP status before upgrading.
| return ws.close() | |
| return ws.close(1013, 'Rate limit exceeded') |
| this.addMessageHandler( | ||
| ClientMessageType.INPUT, | ||
| this.handleInputMessage.bind(this) as MessageHandler | ||
| ) | ||
| this.addMessageHandler( |
There was a problem hiding this comment.
The as MessageHandler casts here weaken type-safety and can hide mismatches between ClientMessageType and the handler payload types. Consider typing the handler map as a discriminated/mapped type keyed by ClientMessageType (or using a switch in onMessage) so handlers can accept their specific message shapes without assertions.
| - name: Setup pnpm | ||
| uses: pnpm/action-setup@v4 | ||
| with: | ||
| version: latest |
There was a problem hiding this comment.
CI installs pnpm with version: latest, which can introduce unexpected breakages when pnpm releases new majors/minors. Pin pnpm to a known-good version (or at least a major) to keep CI reproducible with the checked-in lockfile.
| version: latest | |
| version: 9 |
| WebSocket, | ||
| us_listen_socket, | ||
| us_socket_context_t, | ||
| } from 'uWebSockets.js' |
There was a problem hiding this comment.
WebSocket appears to be used only as a TypeScript type here. Importing it as a runtime named export ({ WebSocket }) can break at runtime if uWebSockets.js doesn’t actually export WebSocket (many libraries only declare it in .d.ts). Please switch to a type-only import for WebSocket (and keep only runtime exports in the value import).
| WebSocket, | |
| us_listen_socket, | |
| us_socket_context_t, | |
| } from 'uWebSockets.js' | |
| us_listen_socket, | |
| us_socket_context_t, | |
| } from 'uWebSockets.js' | |
| import type { WebSocket } from 'uWebSockets.js' |
| { files: ["**/*.{js,mjs,cjs,ts,mts,cts}"], plugins: { js }, extends: ["js/recommended"], languageOptions: { globals: globals.browser } }, | ||
| tseslint.configs.recommended, |
There was a problem hiding this comment.
eslint.config.ts is using flat-config style (defineConfig) but mixes in legacy config fields: plugins: { js } and extends: ["js/recommended"]. @eslint/js exports config objects (e.g. js.configs.recommended) rather than a plugin, and flat config extends should reference config objects (not strings). As written, ESLint 10 will likely fail to load or silently not apply the intended rules—please restructure the config to compose js.configs.recommended + the TypeScript ESLint configs directly.
| { files: ["**/*.{js,mjs,cjs,ts,mts,cts}"], plugins: { js }, extends: ["js/recommended"], languageOptions: { globals: globals.browser } }, | |
| tseslint.configs.recommended, | |
| { | |
| ...js.configs.recommended, | |
| files: ["**/*.{js,mjs,cjs,ts,mts,cts}"], | |
| languageOptions: { | |
| ...js.configs.recommended.languageOptions, | |
| globals: { | |
| ...(js.configs.recommended.languageOptions?.globals ?? {}), | |
| ...globals.browser, | |
| }, | |
| }, | |
| }, | |
| ...tseslint.configs.recommended, |
| import { defineConfig } from "eslint/config"; | ||
|
|
||
| export default defineConfig([ | ||
| { files: ["**/*.{js,mjs,cjs,ts,mts,cts}"], plugins: { js }, extends: ["js/recommended"], languageOptions: { globals: globals.browser } }, |
There was a problem hiding this comment.
This backend ESLint config sets languageOptions.globals to globals.browser, which omits Node globals like process/Buffer. That will cause no-undef (or similar) errors throughout the server code. Use Node (or a merge of Node + ES globals) for backend files, and only apply browser globals to frontend code if needed.
| { files: ["**/*.{js,mjs,cjs,ts,mts,cts}"], plugins: { js }, extends: ["js/recommended"], languageOptions: { globals: globals.browser } }, | |
| { files: ["**/*.{js,mjs,cjs,ts,mts,cts}"], plugins: { js }, extends: ["js/recommended"], languageOptions: { globals: globals.node } }, |
| import js from "@eslint/js"; | ||
| import globals from "globals"; | ||
| import tseslint from "typescript-eslint"; | ||
| import { defineConfig } from "eslint/config"; | ||
|
|
||
| export default defineConfig([ | ||
| { files: ["**/*.{js,mjs,cjs,ts,mts,cts}"], plugins: { js }, extends: ["js/recommended"], languageOptions: { globals: globals.browser } }, | ||
| tseslint.configs.recommended, | ||
| ]); |
There was a problem hiding this comment.
The new TS config file doesn’t follow the repository’s Prettier settings (single quotes, no semicolons, line width). This will introduce avoidable formatting diffs when the file is auto-formatted. Please run Prettier (or adjust the file) to match .prettierrc.
| import js from "@eslint/js"; | |
| import globals from "globals"; | |
| import tseslint from "typescript-eslint"; | |
| import { defineConfig } from "eslint/config"; | |
| export default defineConfig([ | |
| { files: ["**/*.{js,mjs,cjs,ts,mts,cts}"], plugins: { js }, extends: ["js/recommended"], languageOptions: { globals: globals.browser } }, | |
| tseslint.configs.recommended, | |
| ]); | |
| import js from '@eslint/js' | |
| import globals from 'globals' | |
| import tseslint from 'typescript-eslint' | |
| import { defineConfig } from 'eslint/config' | |
| export default defineConfig([ | |
| { | |
| files: ['**/*.{js,mjs,cjs,ts,mts,cts}'], | |
| plugins: { js }, | |
| extends: ['js/recommended'], | |
| languageOptions: { globals: globals.browser }, | |
| }, | |
| tseslint.configs.recommended, | |
| ]) |
| "overrides": { | ||
| "minimatch": "^10.2.1" | ||
| "minimatch": "^10.2.1", | ||
| "jiti": "latest" |
There was a problem hiding this comment.
jiti is overridden to "latest", which makes installs non-reproducible and can break CI unexpectedly when a new major is released. Prefer pinning to a specific version/range (matching the lockfile) or removing the override unless there’s a concrete compatibility need.
| "jiti": "latest" | |
| "jiti": "^2.6.1" |
| this.addMessageHandler( | ||
| ClientMessageType.PROXIMITY_PROMPT_INTERACT, | ||
| this.handleProximityPromptInteractMessage.bind(this) | ||
| this.handleProximityPromptInteractMessage.bind(this) as MessageHandler | ||
| ) | ||
| this.addMessageHandler( |
There was a problem hiding this comment.
These casts to MessageHandler remove compile-time guarantees that the handler matches the message type. A typed handler map keyed by ClientMessageType would keep this safe without as assertions.
| this.addMessageHandler( | ||
| ClientMessageType.SET_PLAYER_NAME, | ||
| this.handleSetPlayerNameMessage.bind(this) | ||
| this.handleSetPlayerNameMessage.bind(this) as MessageHandler | ||
| ) | ||
| } |
There was a problem hiding this comment.
Same issue as above: the as MessageHandler casts hide mismatches between the message discriminator and the handler signature. Prefer a typed mapping (or narrowing in onMessage) so the compiler enforces correct handler/message pairing.
No description provided.