Repository navigation
fix(web): surface accessible illegal-move feedback - #87
Merged
Merged
Conversation
BoardInteraction turned an on-turn destination the LegalMoveOracle does not offer into 'deselect', so a rejected move was indistinguishable from an ordinary deselection. It now returns a typed 'illegal' result; premoves, reselection, deselection, promotion and server authority are unchanged. BoardView forwards it through a separate exhaustive onFeedback channel and the game route renders it in its own polite live region (#move-feedback), not the controller-owned #status. Each rejection is a fresh node so repeats are re-announced; focus never moves; the message clears on any other gesture, position, turn or disposal. setTurn now refreshes a selection that survives the turn arriving, so a legal move is never reported illegal.
|
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 |
PR Summary by QodoSurface accessible feedback for locally rejected chess moves
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- A finished game is nobody's turn: 'ended' empties legal moves but keeps turn, so a resignation or flag on our turn made every attempt 'illegal'. - BoardView.setTurn re-renders a selection refreshed by the turn arriving. - The rejection also clears on 'none' results and when a drag starts. - Use the existing .error (Ember) primitive for the message. - Topology guard: 31 discovered Playwright specs; the new one is backend-only.
Owner
Author
|
/review |
|
Code review by qodo was updated up to the latest commit de4ce7b |
- Qodo on de4ce7b: myTurn=false means premove, so a finished board still selected pieces, queued premoves and wrote 'Premove set' over the result (on main already for endings on the opponent's turn). BoardInteraction gains setInputEnabled; the game route sets it from isOver, which drops selection, premoves and a pending promotion. - setTurn and setInputEnabled re-rendered on every sync/action update, detaching cells under the board-geometry e2e (7/20 with the shield off); they now render only when something visible changes.
Owner
Author
|
/review |
|
Code review by qodo was updated up to the latest commit 5cb1485 |
Qodo on 5cb1485: disabling input mid-drag left the floating piece following the pointer until release. setInputEnabled(false) now removes the float and forgets the drag, so later pointer events find nothing to move or release.
Owner
Author
|
/review |
Owner
Author
|
@greptileai review |
|
Code review by qodo was updated up to the latest commit 069cd6c |
…ed drags - Greptile on 069cd6c: mountBoard.setInputEnabled cleared the rejection on every action-state update (draw offer, connection blip). It now returns early unless input actually changes; that is also the single churn guard, so the view's duplicate check is removed. - Qodo on 069cd6c: abandoning a drag (game end or destroy) left its window pointermove/pointerup listeners attached. A shared cancelDrag removes the listeners, the floating piece and the drag state.
…eedback # Conflicts: # docs/PROJECT_STATE.md
Owner
Author
|
/review |
Owner
Author
|
@greptileai review |
|
Code review by qodo was updated up to the latest commit 8e556a3 |
edwardnewgate710
pushed a commit
that referenced
this pull request
Oct 3, 2026
…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.
sayed710
added a commit
that referenced
this pull request
Oct 4, 2026
* fix(web): enforce player ownership on board interaction 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. * docs: record M15 Increment 87 player board ownership * fix(web): drop the trailing click of a gesture cut short by an owner 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. * docs: record the Increment 87 review correction and its validation * fix(web): clear click suppression when the release that set it makes 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. * docs: record the Greptile correction and Linux backend validation for Increment 87 * docs: correct Increment 87 over-claims and record the exact-head review * fix(web): tie click suppression to the pointer that produced the click 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. * docs: record the pointer-matched suppression fix and its Linux validation * fix(web): bound the no-pointer-id click fallback and ignore other pointers' 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. * docs: record the bounded click fallback and its Linux validation * fix(web): end a drag on its own pointercancel and on a new press 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. * docs: record the drag cancellation fix and its Linux validation * fix(web): a new press abandons another pointer's gesture without acting 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. * docs: record the abandoned-gesture fix and its Linux validation * fix(web): track abandoned-gesture waits and click suppressions per pointer 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. * docs: record per-pointer gesture tracking and its Linux validation * fix(web): tie a pointer-less click to the last released pointer Greptile on 3bd6965: on an engine whose clicks carry no pointerId, the fallback matched clicks with a press count shared by all fingers. If finger A was abandoned after finger B pressed and A released without a click, A's suppression was recorded at B's press count and B's genuine tap was swallowed. A click without a pointer id is now taken to be the last released pointer's (a browser dispatches a click straight after its own pointer's release) and is swallowed only if that pointer is suppressed; the shared press counter is gone. An awaited pointer's pointercancel now ends its wait without recording a suppression, since a cancelled pointer never clicks (the optional LOW from the independent review of 3bd6965). Engines with pointer ids are unchanged. * docs: record the last-release click fallback and its Linux validation * fix(web): bound click records and put abandoned clicks first on engines 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. * docs: record bounded click records and the safety-first pointer-less click rule * fix(web): only recent releases can claim a pointer-less click 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. * docs: record time-bounded pointer-less click matching and its Linux validation * test(web): pin the pointer-less click window from both sides 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. * docs: record the pinned click-window tests and their Linux validation * docs: record the merge with #89 and its Linux validation * fix(web): an abandoned release swallows every pointer-less click in its 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. * docs(web): say when a suppressed click keeps its record * docs: record the abandoned-click fix and its Linux validation * docs: keep one current Increment 88 line in the project-state header --------- Co-authored-by: Hussein Mohamed <americanopbr@gmail.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.
Root cause (reverified on
origin/main2dd6d4d)On our turn,
BoardInteraction.attemptturned a destination theLegalMoveOracledid not offer into{ kind: 'deselect' }. That is the same result as tapping the selected square again, so a rejected move was silent for mouse, touch, drag and keyboard alike. Server rejection presentation was already separate and is unchanged.Contract
GestureResult{ kind: 'illegal', from, to }. It is returned only on our turn, when a selected or dragged own piece is sent to a destination absent fromoracle.destinations(from)and the target is not another own piece. It carries no reason (the oracle exposes none).none: empty or opponent taps with nothing selected, and any gesture while a promotion is pending.deselect: the same square again, or a drop on the source.select: reselection by tap or drop.premove: every off-turn destination.applyPremove.setTurnnow refreshes a selection that survives the turn arriving without a new position (readiness, an acknowledged move). Otherwise a legal move would have been reported illegal.BoardView: an exhaustive switch (with aneverdefault) and a separate typedonFeedback(MoveFeedback)channel.onResultstill carries only moves to submit or queue, becausemountBoardtreats any non-move as a premove. There is no per-input legality code.mountBoard: an optionalfeedbackEl. The game route passes#move-feedback, arole="status"polite region. It is separate from#status, whichGameControllerrewrites on every sync.board.feedback.illegalMove= "That move isn’t legal." (English only, no coordinates). It reuses the mutedcountstyle: no animation and no colour-only signal.Server authority
A rejected gesture never calls
onMove, sends no WebSocket frame and creates no optimistic state; legal moves are submitted exactly as before. There is no API, socket, gateway, persistence or migration change.Tests
2dd6d4dwith the tests compiling: 5 interaction and 8 board tests failed on behaviour.interaction.test.ts): tap and drag rejection, an uncapturable opponent piece, a move straight after, two oracles on the same gesture, a selection surviving the turn arriving, reselection by drop, off-turn premoves, and a pending promotion.board-a11y.test.ts, realmountBoard+BoardView):onMove;e2e-live-loop.test.ts, realGameAuthority):illegal-move-feedback.spec.ts):moveframe, the board and#statusare unchanged, and a repeat is a new node. The next legal move commits ("Black to move") and sends exactly one frame. A spectator's gestures send nothing and show nothing.dir="rtl": no overflow, and the board stays at least 350 px wide.Exact-head review corrections (
de4ce7b)Qodo found 3 bugs and Greptile 2 P2 findings on
6a8257d; all were valid and fixed with RED tests first:endedempties legal moves but keepsturn, so after a resignation or flag on our turn every attempt read "not legal".BoardView.setTurnre-renders a selection that the turn's arrival refreshed.noneresults and when a drag starts..error(Ember) primitive, which the design rules reserve for errors.CI also failed: the topology guard pins the Playwright spec count. It is now 31, with the new spec asserted backend-only. My first local
test:scriptsrun predated the spec file.Second correction (
5cb1485)de4ce7b: a finished board still accepted premoves.myTurn = falsemeans "premove", and the route passes noplayerColor. On main this already happened for endings on the opponent's turn.BoardInteraction.setInputEnablednow turns input off for a finished game. The route sets it fromisOver, which drops the selection, premoves and any pending promotion.Self-introduced regression, caught by the final gate. The new
setTurn/setInputEnabledrenders rebuilt the grid on every sync and action update, which detached cells under the board-geometry e2e. With the shield off it reproduced in 7 of 20 runs. Both now render only on a real change; the same set passed 20 of 20, and a unit test pins the behaviour.Third correction (
069cd6c)Qodo on
5cb1485(it also marked the premove finding resolved): a game ending mid-drag left the floating piece following the pointer until release.setInputEnabled(false)now removes the float and forgets the drag. A RED test came first, and a mutation removing the fix is killed. On this tree the final gates passed first time.Fourth correction (
0891257)On
069cd6c, Greptile (5/5) found one issue and Qodo one bug; both were valid and fixed with RED tests first:destroy) left its windowpointermove/pointeruplisteners attached. A sharedcancelDragnow removes the listeners, the floating piece and the drag state.Integration with main
PR #88 merged first as
f99a9e1and took M15 Increment 85, migration 0048 and ADR-0155. I merged it normally ina0afb83, with no rebase or force.docs/PROJECT_STATE.md. fix(trust): add durable retry isolation for terminal analysis #88's entry is byte-identical to main's, and this PR's entry is now Increment 86.Recreatetrust-worker deployment and the retry/lease code are all present.Falsification
Final tree: 25 mutations, with sources backed up and restored by SHA-256, and none killed by compile errors.
The original 13 were:
setTurnrefresh removedValidation
All runs were sequential, on the final integrated tree
8e556a3:check:*,check:test-topologytest:scripts--retries=0--workers=4 --retries=0On the integrated tree both gates passed on the first run. On the earlier
5cb1485tree the first backend run was invalidated, not failed:vite previewcrashed natively (0xC0000409), leaving 17 passed and 214ECONNREFUSED. The owner authorized exactly one unchanged rerun, which passed 231/231. Avast Web/Network Shield was off for the final browser runs only and is back on.Not included
The PR stays open for the owner to merge.