fix(web): enforce player ownership on board interaction - #90
Merged
Merged
Conversation
BoardInteraction fell back to the side to move when no player colour was given, and the game route mounted its board without one and enabled input whenever the game was not over. Spectators could select, drag and premove; the board was interactive before the joined role arrived; an off-turn player could pick up the opponent's pieces and queue premoves with them; and a finished-game join briefly enabled input. - Ownership is explicit: a colour, null (nobody) or 'side-to-move' (boards without players only). setPlayerColor drops the selection, a pending promotion and queued premoves on a real change; drop() re-checks the origin's owner. Legality stays oracle-driven for every variant. - BoardView.setPlayerColor closes an open promotion chooser and abandons a drag; focus, keyboard navigation and flip are untouched. mountBoard exposes it with the same churn guard as setInputEnabled. - The game route mounts fail-closed and derives owner and input from one GameSync snapshot from both onColor and onActionState, so callback order cannot open a window; a finished game latches. Tests: core, view and route suites plus a backend Playwright spec with a real two-player game and a spectator (click, keyboard and drag). The finished-board step of illegal-move-feedback.spec.ts relied on the old fallback and now tries White's own premove.
PR Summary by QodoEnforce player ownership for game-board interactions
AI Description
Diagram
High-Level Assessment
Files changed (12)
|
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
 |
…change Qodo on 6068e0b: BoardView.setPlayerColor cancelled an in-progress pointer gesture, which removed its pointer-up handler, so the gesture's trailing click reached tap() and could select the new owner's piece (e.g. a swipe begun before `joined` landed). An owner change during a gesture now suppresses that click, and every pointerdown resets the suppression so an abandoned gesture cannot swallow the next real click.
Collaborator
Author
|
/review |
Collaborator
Author
|
@greptileai review |
|
Code review by qodo was updated up to the latest commit a623b11 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…no click Greptile on a623b11: a gesture cut short by an owner change and released off the board left suppressClick set, so the next click with no pointer events (assistive technology, programmatic activation) was swallowed. An ordinary drag dropped off the board leaked the same way. Suppression now covers only the click the release itself produces: it clears on the next tick, since the browser dispatches that click straight after pointer-up. An owner change waits for the cut-short gesture's release (or pointercancel) instead of setting a sticky flag; the wait ends at that release, at the next pointerdown, or on destroy.
Collaborator
Author
|
/review |
Collaborator
Author
|
@greptileai review |
|
Code review by qodo was updated up to the latest commit 38e1bdd |
Qodo and Greptile on 38e1bdd: clearing suppression on a next-tick timer assumed a release's click arrives in the same task, which touch does not guarantee, so a late touch click from a completed drag or from a gesture an owner change cut short could still act. Qodo also found that the cut-short wait ended on any pointer's release, so a second finger lifting first let the original finger's click through. Suppression now names the pointer whose next click must be swallowed and matches the click's own pointerId whenever it arrives. A click with no pointer behind it (pointerType '', e.g. assistive technology) is never swallowed. The wait ends only on that pointer's release or cancel, a new press by the same pointer, or destroy; a new press by the same pointer also clears a stale entry from a release that made no click. Engines whose clicks carry no pointer fields keep suppressing the release's own click.
…nters' drag releases Independent exact-head review of 79686a5: - On an engine whose click carries no pointerId, a stale suppression (e.g. a touch drag released off the board, which makes no click) matched the next tap's click and swallowed it once. In that fallback a click now counts as the release's own only if no press came after the release (a press counter, not a timer). Engines with pointer ids are unchanged. - The drag's pointerup listener accepted any pointer, so another finger's release could drop the carried piece at its own coordinates. It now ignores releases from other pointers.
Independent exact-head review of 066f046: drags never listened for pointercancel, so a touch drag the browser took over (a pan, a system gesture) stayed live with its floating piece; after the drag's pointerup started ignoring other pointers, no later event recovered it. A new press during a live drag also dropped the old drag's listeners without removing its floating piece. A drag now cancels on its own pointer's pointercancel, like a drop off the board (selection cleared, floating piece removed). A press that never became a drag keeps an existing selection. A new press calls cancelDrag() before starting, so no floating piece is left behind.
…ng on it Independent exact-head review of dd52355: a press during another pointer's live drag only detached that drag's listeners. Its selection stayed, its release went unwatched, and its trailing click reached tap() with the piece still selected, so an abandoned drag could submit a move. Owner changes and new presses now share abandonGesture(): wait for the abandoned pointer's release and swallow its click, and undo a drag it started (selection cleared, re-rendered). A re-press by the same pointer (its release was lost) only resets, so its own click still works.
…inter Independent exact-head review of b1441a4: a single wait slot and a single suppression slot meant a third simultaneous pointer could defeat them (abandoning A replaced the pending wait on C, so C's click still tapped), and setInputEnabled(false) still only cancelled a drag. Waits are now a set of awaited pointer ids served by one window listener pair (attached while any is awaited), and suppressions a map of pointer id to the press count at its release. Each pointer's release, click, re-press and the legacy no-pointer-id fallback work as before, per pointer. setInputEnabled(false) now abandons the gesture like an owner change. The PR #87 test that asserted no pointerup listener after a game ends now asserts the new contract: drag listeners go at once, and only the wait for the abandoned release remains until that release.
Collaborator
Author
|
/review |
Collaborator
Author
|
@greptileai review |
|
Code review by qodo was updated up to the latest commit 8314d11 |
…es without pointer ids Qodo on 8314d11: off-board drops (and, earlier, cancelled pointers) recorded click suppressions that no click could ever consume, and touch pointer ids are never reused, so records grew for the life of the board. Releases off the board now record nothing, and pending records are capped (oldest evicted), which also bounds on-board touch drags that make no click. Greptile on 8314d11: on an engine whose clicks carry no pointerId, finger A's abandoned gesture could produce a delayed click after finger B's release, and the last-released rule attributed it to B, so it acted. Without a pointer id a click cannot be attributed, so a choice is unavoidable: safety first. While an abandoned gesture's click is pending, a pointer-less click is taken to be that one (an abandoned gesture never acts; at worst one genuine tap is swallowed and repeated). Drag-release suppressions keep the last-released rule. Engines with pointer ids are unchanged. Tests now create stale entries with on-board no-click releases so every protection stays reachable; a redundant re-add line and a dead write are removed.
Independent exact-head review of 39c2f9d: on an engine whose clicks carry no pointerId (reportedly including Safari/iOS, unverified), an abandoned touch that never produced a click left an abandoned record with no expiry, so each later genuine tap was swallowed against one stale record, up to the cap: dead taps spread over any amount of time. Records now carry their release's time, and on the pointer-less path records older than CLICK_WINDOW_MS (1 s: a release's click comes within milliseconds, or ~300 ms when touch holds it for double-tap detection) are dropped before matching. Times come from the events' own timeStamp. Engines with pointer ids are unchanged.
Independent exact-head review of 242ad1c: only one test used explicit event times, so shrinking CLICK_WINDOW_MS (e.g. to 50 ms) or flipping its comparison went unnoticed. An abandoned release's click 300 ms later must still be swallowed, and a click just past the window must act.
Resolves docs/PROJECT_STATE.md only: #89's M15 Increment 87 entry and header lines are kept exactly; this PR's entry and its own header lines are renumbered to M15 Increment 88.
Collaborator
Author
|
/review |
Collaborator
Author
|
@greptileai review |
|
Code review by qodo was updated up to the latest commit 431df30 |
…ts window On engines whose clicks carry no pointer id, a newer finger's click could use up an abandoned finger's record before that finger's delayed click arrived, letting the abandoned click act. The record now stays until its window closes, so both ambiguous clicks are swallowed in either order.
Collaborator
Author
|
/review |
Collaborator
Author
|
@greptileai review |
|
Code review by qodo was updated up to the latest commit cbb06a2 |
Collaborator
Author
|
/review |
Collaborator
Author
|
@greptileai review |
|
Code review by qodo was updated up to the latest commit 33736e6 |
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
2 participants
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.
Summary
Spectators could select, drag and premove on the game board. The board was interactive before the server confirmed the player's role. An off-turn player could pick up the opponent's pieces (the side to move) and queue premoves with them. Joining a finished game briefly enabled input. No illegal move reached the server, because
GameSync.submitMovealready refused them, but the board offered gestures it had no right to.Root cause:
BoardInteraction.movableColor()fell back tosideToMovewhen noplayerColorwas given. The game route mounted its board without one and enabled input whenever!isOver.Behavioural contract
nullowner): no selection, drag, drop, keyboard move, promotion or premove, so zero submissions. There is no "illegal move" message either. Focus, roving keyboard navigation and flipping still work.onColorandonActionStatearrive in.Design
core/interaction.ts: explicitBoardOwner = Color | null | 'side-to-move'. The last value is kept only for boards without players (analysis, studies, endgame, learning), which are unchanged.setPlayerColordrops the selection, a pending promotion and premoves on a real change.drop()re-checks the origin's owner.ui/board-view.ts:setPlayerColoralso closes an open promotion chooser and abandons a drag.app/board.ts: aplayerColormount option andsetPlayerColor, with the same churn guard assetInputEnabled.app/game-mount.ts: mounts withnull. OnesyncBoardOwnership()readsmyColorandstatus.overfrom a singleGameSyncsnapshot and is called from bothonColorandonActionState.Tests
board-ownership.test.ts, 14): spectators; fail-closed startup; White-only and Black-only on and off turn, including drops; own premove queued and applied; promotion premove; reselection; illegal feedback for a real player; finished board; owner changes clearing selection, promotion and premoves; Chess960; standalone board.board-a11y.test.ts, +7): spectator click, keyboard and drag submit nothing and announce nothing; navigation, focus and flip on a read-only board; own colour only by click, keyboard and drag; owner change mid-drag and with the chooser open; churn guard; remount.board-ownership-route.test.ts, 10, realmountGamewith a fake socket): before the socket opens and before join; spectators across syncs; legal move; off-turn White and Black; finished join for every role; finished wins; Chess960.board-ownership.spec.ts): a real two-player game plus a spectator, with click, keyboard (Enter and Space) and drag. Nomoveframes come from spectator or opponent-coloured attempts; own premoves queue without being sent; legal moves commit.illegal-move-feedback.spec.ts: the finished-board step relied on the old fallback and would now pass vacuously, so it tries White's own premove instead.d5af5be: the route tests compile and 8 of 9 fail on assertions. The browser spec, run against main's sources, fails because off-turn Black queued a premove of White's e2–e4.playerColor: nullat mount) is equivalent:controller.start()nulls the owner synchronously insidemountGame. The option is kept as defense in depth.Validation (exact head)
check:*guards,test:scripts312--retries=0; Avast Web/Network Shield off at the owner's direction)--retries=0, normal host state with the shield on). Ports were checked free first, and a read-only commit sampler showedvite previewalive from start to teardown.docs/PROJECT_STATE.md(Increment 87): 231/232 (Chromium launch fault); two runs invalidated by the separatevite preview0xC0000409crash (that diagnostic closed with no product defect and no proven root cause); 230/232 (achievements loopback timeout plus a Chromium launch fault). No config, timeout, retry or worker changes were made.Independent review
PROJECT_STATE).6068e0b: Gemini was 429 (resets in about 98h) and Sonnet 4.6 was also 429 (resets in about 1h33m). Per the review hierarchy this fell back to a strict self-review: it re-checked all six earlier findings and every factual claim in Increment 87, and found no blocking issues.Out of scope (pre-existing, disclosed)
setTurn(false), which means premove mode, not read-only.vite previewnative crash.Do not merge from automation: the owner merges manually.