Show DD-table solve time and debounce hand-edit solves - #366
Conversation
Measure calc_table with performance.now() and display "Solved in X ms." in the SE status line so solve cost is visible next to the tricks. Co-authored-by: Cursor <cursoragent@cursor.com>
Sync WASM ccall was freezing the UI on every keystroke; delay those solves, but still run immediately when the deal first becomes complete (auto-fill or final pip). Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🟡 Changes recommended
Debouncing is currently applied even when the deal becomes incomplete/invalid, which delays clear_results() and can leave stale DD-table/status content visible for up to the debounce interval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the DDS web UI to (1) display wall-clock solve time under the DD results table and (2) debounce expensive WASM DD-table solves during hand editing to keep typing responsive, while still solving immediately on “deal becomes complete” and on contract selection.
Changes:
- Add solve-time formatting and display (
Solved in X ms.) after successful DD-table solves. - Introduce a configurable debounce for hand-edit-triggered solves, while preserving immediate solves for contract clicks and first-time deal completion.
- Update unit/E2E tests and UI layout/CSS to support the new status line placement and behavior.
File summaries
| File | Description |
|---|---|
| web/tests/test_web_e2e.py | Updates E2E expectation to assert the solve-time status text is shown. |
| web/tests/dds_web_test.mjs | Adds unit tests for debounce behavior and solve-time formatting/display; updates test harness to provide timers/performance. |
| web/dds_web.js | Implements debounced scheduling and solve-time measurement/formatting; updates refreshDdTable() to display elapsed time. |
| web/dds_web.html | Moves #result under the DD result table and adds aria-live="polite". |
| web/dds_web.css | Styles the #result status line under the table. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Debounce only applied within the same completeness state so stale numerals no longer linger for the debounce window after a card is deleted. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🔵 Needs a closer look
There’s an exported/awaitable API ambiguity in scheduleDealSolveDebounced() (returned promise doesn’t include the debounced work) plus a misleading new test comment/sleep that should be corrected.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
web/dds_web.js:130
scheduleDealSolveDebounced()returnssolveQueue, but that promise does not include the work scheduled by the debounce timer. If any caller ever awaits the return value, it can observe completion before the debounced solve has actually run. Consider either (a) returning a promise that resolves/rejects with the eventual trailingscheduleDealSolve()call (including when superseded/cancelled), or (b) making this function explicitlyvoid/internal (and avoid exporting it) so it can’t be misused as an awaitable API.
web/tests/dds_web_test.mjs:745- This test comment says the initial
fillFormWithPartScoreTestData()solve is debounced, but the production logic solves immediately on the first transition to a complete deal. The 250ms sleep is also longer than needed for the intent of this test (contract click should be immediate regardless of debounce).
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
…test. Returning solveQueue implied the trailing debounced work had finished; stop returning a promise and drop the export. Also correct the fillForm wait that assumed that path was debounced. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the latest Copilot notes in 49675f5:
|
Match the void usage used elsewhere so the returned Promise is clearly ignored. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The new debounce-configuration setter should cancel any already-scheduled debounce timer when disabling debounce to avoid unexpected trailing solves.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
web/dds_web.js:112
setDealSolveDebounceMs()updates the debounce delay but does not cancel any already-scheduled debounced solve. If a caller disables debounce (sets it to 0) while a timer is pending, that trailing solve will still fire later, which is surprising for a setting function and can cause redundant work.
Consider clearing any pending debounce timer when switching to a non-positive delay.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
setDealSolveDebounceMs(0) previously left a trailing solve scheduled at the old delay, which was surprising and could cause redundant work. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the latest Copilot note in the follow-up commit: |
There was a problem hiding this comment.
🔵 Needs a closer look
The current debounce routing also delays non-WASM “clear/error” updates for still-invalid/incomplete edits, which can leave stale validation/status visible until the debounce window elapses.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
web/dds_web.js:2103
- The debounce decision is based on
dealComplete !== lastDealWasComplete, which means edits that keep the deal incomplete/invalid also go throughscheduleDealSolveDebounced(). SincerefreshDdTable()does not run WASM in those states (it only clears results / updates the validation error), debouncing here can leave stale validation text (e.g., which duplicated cards) or other cleared state visible until the debounce window elapses. Consider debouncing only when the deal is solvable (13 cards + valid), and schedule immediately otherwise so clears/errors stay responsive without reintroducing sync-ccall freezes.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Incomplete or invalid edits schedule immediately so clear/error status stays responsive without waiting for the debounce window. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the latest Copilot note: debounce now applies only while the deal stays solvable ( |
There was a problem hiding this comment.
🟢 Approval recommended
The debounce behavior, immediate-solve exceptions, and solve-time UI/status updates are implemented consistently and are covered by targeted unit and E2E tests.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
Solved in X ms.ccalldoes not freeze typingTest plan
bazelisk test //web:dds_web_js_test#resultshowsSolved in N ms.under the DD tableMade with Cursor