Import deals from file and show Computing… on slow DD solves

[tameware]

Summary

• Add an Import deal… button that loads the first deal from PBN, LIN, DLM, dtest .txt, or sol-style lines (e.g. sol10.txt) into the diagram.
• Show Computing… under the DD matrix after a 300 ms grace period, painted before the blocking WASM ccall so long solves are visible (a timer alone never fires during sync WASM).

Test plan

• bazel test //web:dds_web_js_test //web:dds_web_html_test
• python3 web/serve_web.py → open the page, hard-refresh
• Import hands/example.pbn, hands/list1.txt, a .lin, and hands/sol10.txt; confirm the first deal fills and solves
• Import a multi-deal file; confirm only the first deal is used
• Load a slow deal and confirm Computing… appears under the matrix after ~300 ms, then Solved in N ms.

Made with Cursor

[Copilot]

🟡 Changes recommended

Unresolved critical correctness issues remain in stale solve invalidation and LIN dealer handling.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds deal-file importing and visible progress feedback for slow double-dummy solves.

Changes:

• Supports PBN, LIN, DLM, dtest, and sol-style formats.
• Adds an import button and file input.
• Shows “Computing…” after a 300 ms grace period.
• Adds parser, UI, and timing tests.

File summaries

File	Reviewed changes	Findings
web/tests/test_web_html.py	Import-control markup tests	None noted
web/tests/dds_web_test.mjs	Parser and solve-status tests	None noted
web/dds_web.js	Import parsers, file handling, and solve-status flow	Critical stale-solve invalidation and LIN dealer handling issues; moderate deal validation issue; nit-level error messaging and browser file-selection coverage issues
web/dds_web.html	Import button and file input	None noted

Review details

Suppressed comments (2)

web/dds_web.js:696

• The browser file-selection path is not covered: the tests call importDealFromText directly and only inspect the HTML markup, so regressions in input.files[0], File.text(), or surfacing read/parse errors would pass. Add a JavaScript test for handleDealFileSelected with a fake File covering success and failure.

    const result = document.getElementById("result");
    try {
        const text = await file.text();
        const err = importDealFromText(text);

web/dds_web.js:433

• This shared return path only checks that each direction exists and normalizes strings; it never verifies the four suit fields or uniqueness of the 52 cards. Because normalizeHandHolding filters non-pips first, malformed input with repeated cards or extra/missing suit separators can be reported as a successful import and then sanitized by fillFormWithTestData, leaving altered or incomplete input instead of an import error. Validate the assembled deal before returning it.

        if (!byDirection[direction]) {
            return null;
        }
        deal[direction] = normalizeHandHolding(byDirection[direction]);

• Files reviewed: 4/4 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Unresolved deal validation, stale-status, and asynchronous file-selection issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

web/dds_web.js:700

• The new browser file-selection path is not covered by the added tests: all import tests call importDealFromText directly, and the HTML test only checks markup. A regression in file.text(), files[0], or the async success/error handler would therefore pass CI while the Import deal button is unusable. Add a unit or browser test that supplies a file through the actual input and verifies both successful import and read/parse errors.

web/dds_web.js:2664

• These guards reject stale WASM work but do not clear stale status. For a complete-deal edit during the wait, updateActionButtons() only arms the 250 ms debounce, so ddTableRequestId can remain unchanged; if the old timer fires, this branch clears the timer and returns while leaving result.innerHTML as Computing… until the debounced refresh starts. Clear the old status when requestId === ddTableRequestId (and apply the same cleanup to the second PBN guard) so the UI does not report computing for the edited deal.

            // An import/edit during the grace wait can change the diagram while
            // this invocation still holds the old PBN; do not solve stale input.
            if (handsToPbn(collectHands()) !== pbn) {
                clearDdTableComputingTimer();
                return;

web/dds_web.js:701

• file.text() is asynchronous, so two quick file selections can complete out of order. This handler imports whichever read resolves last without checking that file is still input.files[0], allowing a slower read of an earlier selection to overwrite the deal chosen afterward. Re-check the current file after the await (or use a selection token) before calling importDealFromText.

        const text = await file.text();
        const err = importDealFromText(text);

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Three moderate issues remain around input validation, stale solve invalidation, and hidden-tab paint handling.

Review details

Suppressed comments (3)

web/dds_web.js:416

• This normalization silently changes malformed input: it pads missing suit components, truncates extras, and sortPips filters illegal rank/seat characters. A PBN/sol hand such as AKQJ.AKQJ.T98.T9.2 can therefore pass the 13-card/52-card checks with data dropped, and an extra compass letter is ignored instead of rejecting the file. Validate each raw hand as exactly four suit components containing only allowed ranks before normalizing, matching the repository's PBN parser contract.

    const parts = String(dotted).split(".");
    while (parts.length < 4) {
        parts.push("");
    }
    return parts.slice(0, 4).map(sortPips).join(".");

web/dds_web.js:751

• A failed file import only overwrites the shared status text here; it does not invalidate an in-flight refreshDdTable(). If the read/parse finishes while that solve is in its async grace or paint phase, the old request can subsequently paint Computing… or Solved in… and hide the import failure. Invalidate the active DD request and clear its computing timer before displaying either file error (the read-error catch below needs the same treatment).

        const err = importDealFromText(text);
        if (err && result) {
            result.innerHTML = err;

web/dds_web.js:2635

• Browsers commonly pause requestAnimationFrame callbacks for hidden/background tabs. In that state this promise can remain pending indefinitely, so refreshDdTable() never reaches the WASM call and the solve queue is blocked until the tab is foregrounded. Skip the paint wait when document.visibilityState is hidden (or add a timeout fallback) so this UI optimization cannot prevent solving.

    if (typeof requestAnimationFrame === "function") {
        return new Promise((resolve) => {
            requestAnimationFrame(() => {
                requestAnimationFrame(resolve);
            });

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Three moderate issues remain in parsing and stale-status handling.

Review details

Suppressed comments (3)

web/dds_web.js:653

• The checked-in hands/sol10.txt format prefixes each deal with a board number (1., 2., …). As written, that prefix becomes a fifth token, so parseSolStyleDealLine returns null for every real sol10 line and the advertised sol-style import fails. Strip the optional <number>. prefix before tokenizing the four hands.

    const hands = beforeColon.split(/\s+/).filter(Boolean);
    if (hands.length !== 4) {
        return null;

web/dds_web.js:568

• parseLinHand silently drops every character that is neither a suit marker nor a rank. Because dealFromDirectionMap validates only the sanitized holding, a malformed LIN hand such as a valid 13-card hand followed by X is accepted and imported as if X were absent. This can hide corrupted files and is inconsistent with the raw-character validation used for PBN; reject unknown or misplaced characters here and propagate that failure from parseLinDealPayload instead of sanitizing them away.

        if (suit && PIPS.includes(upper)) {
            holdings[suit] += upper;
        }

web/dds_web.js:2654

• Normal hand edits do not advance ddTableRequestId or clear this timer: updateActionButtons() only advances the solve epoch, and it does not call clear_results(). If the module load or grace wait is still pending when the user edits the deal, this callback can paint Computing… for the old PBN and leave it visible until that stale refresh resumes, even though no solve for the current diagram is running. Invalidate/clear the timer on every diagram change, or include the current PBN/solve epoch in this callback's guard.

        if (requestId !== ddTableRequestId || !result) {
            return;
        }
        result.innerHTML = "Computing…"; // horizontal ellipsis

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Pending solve debounces and painted Computing status need to be cleared when invalidating requests.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

web/dds_web.js:760

• This invalidates the active DD request but leaves dealSolveDebounceTimer armed. If a user selects an invalid/unreadable file while a complete-deal edit is waiting for the debounce, that timer will later schedule the old deal's solve and overwrite the import error (and possibly the matrix). Cancel the pending deal-solve debounce here as well so the file error remains authoritative.

    ddTableRequestId += 1;
    clearDdTableComputingTimer();
}

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Three unresolved moderate issues need to be addressed before approval.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

web/dds_web.js:2693

• The visibility check only covers the moment before scheduling the two animation frames. If the tab becomes hidden after the first callback, the second requestAnimationFrame can be paused indefinitely, leaving showComputingStatus() and the shared solve queue pending; subsequent edits then cannot solve because dealSolveQueued remains true. Add a timeout/fallback or re-check visibility before scheduling the second frame.

web/dds_web.js:760

• This only invalidates the DD request, but the surrounding scheduleDealSolve job still sees the same dealSolveEpoch. If a complete imported deal has a selected contract, an in-flight refresh can return as stale and then pass the epoch check, immediately running refreshOpeningLeadTricks() before the debounced DD solve for the new deal; a failed import can likewise let that queued lead solve overwrite its error. Invalidate/coalesce the enclosing solve job too, or make it stop after an aborted DD refresh.

function invalidateActiveDdTableRequest() {
    ddTableRequestId += 1;
    clearDdTableComputingTimer();

web/dds_web.js:424

• PBN permits - as the notation for a void holding, and the repository's own convert_from_pbn accepts it as a non-card separator (library/src/pbn.cpp:99-115). This validation rejects the dash, so otherwise valid PBN deals such as ...- cannot be imported; treat a - suit component as empty while continuing to reject other illegal characters.

    for (const part of parts) {
        for (const ch of part) {
            if (!PIPS.includes(ch.toUpperCase())) {
                return false;

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

A moderate issue can let stale solves bypass debounce and overwrite import errors.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Three moderate issues remain, along with one test-isolation nit.

Review details

Suppressed comments (4)

Previously missed (2) — in code that hasn't changed since the last review.

web/dds_web.js:684

• Because \s* also matches zero spaces, this treats a valid first hand whose spade holding starts with a numeric rank (for example 2.AKQJ...) as a board-number prefix. A no-prefix sol-style line then loses its first suit component and is rejected; require the documented whitespace after the optional <n>. prefix or otherwise disambiguate it.
  web/dds_web.js:786
• This invalidates only DD-table requests. If a selected-contract lead solve is still awaiting loadDdsModule() when a file import/read fails, refreshOpeningLeadTricks() can later enter its catch and overwrite the import error in #result because leadTricksRequestId was not advanced. Invalidate the lead-trick request here as well.

web/dds_web.js:612

• This accepts a malformed LIN payload with more than four comma-separated hands: the loop below only reads parts[0..3] and silently ignores any fifth hand, so md|...,...,...,...,...| can import the first four instead of reporting an invalid file. Reject payloads with more than four parts (while retaining the supported 3-part omitted-hand form).

    if (parts.length < 3) {

web/tests/dds_web_test.mjs:1559

• ccallSawComputing is also set by the warm-up solve triggered at line 1552, and it is never reset before the forced refresh. Therefore the final assertion can pass even if the refresh under test never paints Computing…; reset the flag after the warm-up completes so this test isolates the second solve.

    // fillForm schedules a solve; wait for it so it does not race the Act call.
    await new Promise((resolve) => setTimeout(resolve, 350));

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Unconditional delay adds latency to fast solves and displays Computing… unnecessarily.

Review details

Suppressed comments (1)

web/dds_web.js:2827

• Because this await is unconditional, every uncached solve is held for at least ddTableComputingDelayMs (300 ms in production) before ccall, and showComputingStatus() then paints Computing… even when the solve would finish immediately. This adds a visible minimum latency to fast solves and contradicts the nearby claim that the grace period avoids a fast-solve flash; please avoid making the delay unconditional or explicitly choose/document a worker-based or minimum-latency tradeoff.

            const remainingMs =
                ddTableComputingDelayMs - (performance.now() - waitStartedAt);
            if (remainingMs > 0) {
                await new Promise((resolve) => setTimeout(resolve, remainingMs));
                if (requestId !== ddTableRequestId) {
                    return;
                }
            }

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

No unresolved blocking issues were identified.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[zzcgumn]

I am happy for this to be merged but my web knowledge is very small.