Repository navigation
Cover Slides refresh cancellation across editor mounts - #8244
Conversation
|
Here's a visual recap of what changed: Open the full interactive recap |
There was a problem hiding this comment.
Builder reviewed your changes and has a few items to flag 🟡
Review Details
PR #8244 adds an editor-level regression test for a pending null-result deck refresh across unmount/remount, complementing the existing visibility test. This is a low-risk, test-only change, and it does not alter production behavior. The fixture clears the shared refresh mock between mounts, disposes the first router, and verifies the hidden editor does not refresh when timers advance. One review found a narrow coverage gap: the first editor is unmounted after a microtask flush, before the null-result retry's backoff timer is scheduled, so the test does not exercise the cleanup's cancelled guard for that timer. Current production cancellation appears correct; the concern is that the regression does not fully prove its stated retry-across-unmount scenario. The other review found no additional actionable issue. Browser testing: Skipped — this PR only modifies tests and has no UI impact.
|
Checked the timer-ordering concern in the review summary. A temporary timer spy asserted that This case targets the helper's inner null-result retry across unmount, rather than the later outer polling backoff. The earlier negative control removed only the effect's unmount abort and produced exactly one stale refresh; restoring the abort passed. No production or test change is needed for this finding. The dependency-install rerun is green, and the PR description now reflects that.
|

Add an editor-level regression for the 250 ms null-refresh retry surviving unmount and calling the next hidden editor's shared refresh function.
The test mounts an unresolved deck, leaves its first null-result refresh waiting, unmounts it, clears the shared refresh mock, and mounts the deck hidden. Advancing the clock must produce no refresh calls. This checks the editor cleanup wiring as well as the helper's cancellation contract; the existing visibility test covers hiding a mounted editor.
#8193 already implements the AbortSignal repair. The repeated failure in #8147 at
021d52bused a synthetic merge intob7565d8; neither revision contains #8193. This PR adds regression coverage for that mechanism without duplicating the production fix or relaxing the failing assertion. It changes only the Slides 2 owning spec.Validation:
git diff --checkand TypeScript syntax checks passed.Local execution used one worker and a temporary Vitest config resolving workspace exports to source because the restored Windows checkout lacked built package outputs. This is runtime evidence for the owning source, not a package-build result. No assertions were relaxed. Independent technical review found no blocking issues in the published diff.
Hosted CI is now green after rerunning the dependency-install failure. A fresh local probe also asserted that the 250 ms retry timer was registered before the first editor unmounted; the regression passed. The test targets this inner null-refresh retry across unmount, while the outer polling backoff is separate.