Skip to content

Fix PGN parser throwing on ambiguous moves#625

Draft
benckx with Copilot wants to merge 2 commits into
masterfrom
copilot/fix-import-pgn-error
Draft

Fix PGN parser throwing on ambiguous moves#625
benckx with Copilot wants to merge 2 commits into
masterfrom
copilot/fix-import-pgn-error

Conversation

Copilot AI commented May 13, 2026

Copy link
Copy Markdown
Contributor

The PGN parser failed with "Ambiguous move for Cb6" whenever multiple pieces of the same type could legally reach the same square — it had no disambiguation strategy beyond throwing.

Changes

  • Moved ResultCandidate to parser-common.js — extracted from parser-wxf.js so it's available to both parsers (script load order: parser-commonparser-pgnparser-wxf)

  • Replaced single-board approach in PgnParser.parse() with multi-candidate branching — mirrors the existing WXF strategy:

    • 1 candidate → advance normally
    • >1 candidates → fork one ResultCandidate per move; invalid branches are pruned by subsequent moves
    • 0 surviving candidates → throw
  • Annotations preserved — tracked separately per-token index and re-applied to the final result, since ResultCandidate.listMoves() returns null annotations

  • Fixed attemptToAddMove — removed erroneous this.#valid = true on success that could resurrect an already-invalidated candidate

// Before: throws "Ambiguous move for Cb6 (BLACK to play)"
// After: forks candidates for each legal Cb6 source, prunes via future moves
1. Che3 Nhg8 2. Nhg3 Ch2 3. Rih1 Chf2 4. Ra2 Cfb2 5. Cbc3 Cb6 ...

Copilot AI linked an issue May 13, 2026 that may be closed by this pull request
Copilot AI changed the title [WIP] Fix ambiguous import PGN condition Fix PGN parser throwing on ambiguous moves May 13, 2026
Copilot finished work on behalf of benckx May 13, 2026 13:16
Copilot AI requested a review from benckx May 13, 2026 13:16
* Multiple instances are maintained in parallel when a move is ambiguous,
* so that later moves can disambiguate which branch is correct.
*/
class ResultCandidate {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@copilot I think we already have this system in place when parsing WXF moves so that would be redundant

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ResultCandidate was moved out of parser-wxf.js into parser-common.js (commit bce8456 removes 82 lines from parser-wxf.js), so there's no duplication. It needs to live in parser-common.js because the script load order is parser-commonparser-pgnparser-wxf, meaning parser-pgn.js can't reference a class defined in parser-wxf.js.

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.

Import PGN error

2 participants