Take ModalOverlay's closing delay off the wall clock #201
Loading…
Reference in a new issue
No description provided.
Delete branch "deterministic-close-delay"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Tasks 263 (#85) and 261 (#26). Two distinct causes, not one — 263's fix alone did not resolve 261, which was found by measuring rather than assuming.
263 — the closing delay.
ModalOverlay.CloseAsyncnow awaitsIClosingAnimationDelayinstead ofTask.Delay(200). Nothing this application registers supplies one, so production falls through to a realTask.Delayand is unchanged; the only registration anywhere isFakeClosingAnimationDelayin a bUnit context. That is ADR-0160's pattern in C#: the production value is a literal the component owns, and the sole writer of the override is a test, out of band. All 43 tests that used to run the real 200 ms — the 17WaitFor*sites inPlaceFormPanelTestsamong them — now finish the close inside the dispatch that started it, so noWaitFor*in the suite waits on elapsed time. No timeout was raised.CloseAsync_ReducedMotionPreferred_RaisesOnCloseWithNoDelaywas found not to discriminate — deleting the_prefersReducedMotionguard left it green, because a delay that ran and finished is indistinguishable from one that was skipped if you only look afterwards. It now counts the waits asked for, and the ordinary path asserts the count is 1 so the override cannot pass by being ignored.ModalOverlayWallClockDelayTestsis the one fixture that registers no override, and the only place a real delay is still waited out.261 — a different cause, in the same file. With 263's fix in place,
PlaceFormPanel.RequestCloseAsync:587was still bistable, 3 runs in 20. The cause isPlaceFormPanel_CsvImportInFlight_CloseButtonClickIsANoOp: bUnit's synchronousClick()hands the event to the renderer's dispatcher and returns without waiting, and this is the only test in the suite that fires an event while that dispatcher already holds an unfinished work item — the import's own handler. The dismiss ran before the assertions on some runs and only at teardown on others, which the test could not tell apart, every assertion in it being a negative. Awaiting the trigger removes it. Nobody should read 263 as having subsumed 261.Floor to 91.6, which is CI's own measurement. The deterministic figure is 2636 of 2876 = 91.6551%, and reportgenerator's
coveragefield — the number the ratchet actually reads — truncates to one decimal rather than rounding, so it reports 91.6, not 91.7. Same reason this baseline'sPlaceMark.Infrastructure: 99.1is a truncated 99.1667%. Verified end to end against the merged six-report summary throughcheck-coverage-ratchet.cs, not by dividing line counts by hand. No slack.Determinism: 20 post-rebase WebUI coverage runs (12 idle, 8 under 96 spinners on 24 cores) all give 2636/2876 with zero differing lines, on top of 36 pre-rebase runs and 5 full-solution runs in which no covered line differs in any assembly.
Guards watched fail: the fallback removed, the reduced-motion guard deleted,
StateHasChangeddeleted,Hold()made a no-op, the component reverted toTask.Delay, and the_importingguard deleted — one to five named tests reddened each time, restored green after.ADR-0168 corrected, following the pattern PR #200 set on ADR-0071: a
Corrected:front-matter line and two inline> Correctionnotes at the now-false clauses, with the original text left standing.ModalOverlay.ClosingDelaynow carries its own paragraph on why it is a lookup rather than a plain[Inject] IClosingAnimationDelay: a required injection needs aProgram.csregistration, and nothing in this suite would notice that registration going — no bUnit test resolves the application's container, and every test class registers its own override — so deleting the line would leave the suite green and every reader's modal closing with no animation. Keeping the production value as the fallback of a deliberately-absent service means there is no registration to lose.ef793e89354b6eec2f58dbeb8c8a7ab9af9e72e8Verdict: changes needed
Truncation verified independently rather than taken on trust: reportgenerator 5.5.11's
JsonSummarycoveragefield is a truncation, not a rounding. Fed a synthetic cobertura of 2636/2876 and 2380/2400 it emits91.6and99.1, not91.7and99.2. So 91.6 is the right floor, 91.7 would fail the ratchet (0.1 > the 0.05 tolerance), and nothing is left slack. Worth stating plainly: task 261's own description is wrong in the same direction — it attributes 91.4-or-91.5 to the ratchet'sMath.Round(coverage, 1), butMath.Round(91.4734, 1)is 91.5; only truncation gives 91.4. Any future baseline reasoned out by dividing line counts by hand will be up to a decimal high.The optional-service shape itself is sound — the production value stays a literal the component owns, there is no registration to lose,
IServiceProvideris always resolvable in both containers, and the cost is one lookup per close. Its stated rationale is wrong in one detail; see the inline note on ModalOverlay.razor.cs:85.Determinism: 20 post-rebase runs on their own leave ~4% chance of missing a flake that ran at 3-in-20 (0.85^20). The exclusion is comfortable only with the 36 pre-rebase runs included; the 2636/2876 figure itself rests on the post-rebase 20 alone.
The one thing blocking is the tripwire's discrimination (ModalOverlayWallClockDelayTests.cs:49) plus two comment corrections.
@ -1,6 +1,11 @@# ADR-0168: Choose between "Open in maps app" and "Share" by pointer type- **Status:** Accepted- **Corrected:** 2026-08-20, working Vikunja task 261 — this record's coverage note says the WebUINot blocking this PR, and the corrections themselves are accurate — but ADR-0107 permits exactly four status-metadata fields (
Status,Superseded by,Partially superseded by,Answered by) and freezes everything else, and this adds a fifth plus two body insertions. PR #200 did the same an hour earlier, so there are now two records carrying a mechanism no record sanctions, which is the shape ADR-0107's own Alternatives section warns about. Raise a ticket for a short ADR addingCorrected:the way ADR-0107 addedAnswered by, or the rule and the practice stay in disagreement.@ -64,0 +82,4 @@/// <b>Why this is a lookup rather than a plain <c>[Inject] IClosingAnimationDelay</c>, which would/// be the ordinary shape.</b> A required injection needs a registration in <c>Program.cs</c> to/// satisfy it, and that registration would then be the only thing standing between every reader and/// a modal that closes with no animation at all. Nothing could notice it going: no bUnit testThe counterfactual is wrong. A required
[Inject] IClosingAnimationDelaywhoseProgram.csregistration was deleted does not give readers a modal that snaps shut — Blazor's property injector resolves throughGetRequiredService, so the first render of any modal throwsCannot provide a value for property 'ClosingDelay' on type 'ModalOverlay'. Loud, not silent. The conclusion still holds (nothing in this suite exercises the application's container, so the suite would stay green either way), but reword the failure mode: the choice buys "no start-up dependency to lose", not "no silent degradation".@ -64,0 +83,4 @@/// be the ordinary shape.</b> A required injection needs a registration in <c>Program.cs</c> to/// satisfy it, and that registration would then be the only thing standing between every reader and/// a modal that closes with no animation at all. Nothing could notice it going: no bUnit test/// resolves the application's own container, and every test class in the suite registers its own"every test class in the suite registers its own override" is not exact:
PlaceDetailPanelTestsrenders aModalOverlaythroughPlaceImageViewerand registers none. Same overstatement inModalOverlayWallClockDelayTests' class doc ("every other test class in this suite takes that override").@ -0,0 +11,4 @@/// closing wait overridable so no test has to race a real 200ms, and every other test class in this/// suite takes that override. Nothing would then notice if the fallback went — a component that/// resolved an override and had none left to fall back to would close instantly for every reader, the/// animation gone, with all 880 of those tests still green because they never wait on the real clockDrop
880. A test count in a durable comment is stale by the next PR and nothing checks it.@ -0,0 +46,4 @@var closeTask = renderedOverlay.InvokeAsync(() => renderedOverlay.Instance.CloseAsync());closeTask.IsCompleted.ShouldBeFalse();This is the only discriminating line in the whole tripwire, and the mutation that matches the failure this class names — a fallback that no-ops, not one that is deleted — may not redden it.
Dispatcher.InvokeAsynccalled off the renderer's thread is not guaranteed to have started the work item by the next statement, soIsCompletedcan be false because the dispatch has not run yet rather than because a delay pended. Deleting the fallback outright throws an NRE, which any assertion in this method would catch, so "the fallback removed" does not establish this.Mutate
WallClockClosingAnimationDelay.WaitAsynctoTask.CompletedTaskand confirm red. If it stays green, assert a lower bound on elapsed time instead (stopwatch.Elapsed.ShouldBeGreaterThanOrEqualTo(TimeSpan.FromMilliseconds(150))after the await) — a lower bound cannot flake under load, only an upper one can, and this class already accepts paying the real 200ms.Actioned in
ecde08b.#1894 — the tripwire's assertion. Replaced with a lower bound on elapsed time. Your structural objection is right even though the mutation you asked for did redden the old assertion here:
Task.CompletedTaskcaught it 27 of 27 (15 idle, 12 under 96 spinners on 24 cores), so on this machine it discriminated — but "discriminated on this machine" is not the same as "discriminates because a delay pended", and this guard exists for a defect that only ever appeared on a runner I cannot reproduce.Stopwatcharound the awaited close,ShouldBeGreaterThanOrEqualTo(150ms).The new assertion also catches a mutant the old one provably could not: shortening the fallback to
Task.Delay(1)reddens it 10 of 10, whereIsCompletedwould have been false and green. AgainstTask.CompletedTask: red 15 of 15. Unmutated: green 10 of 10, and the WebUI suite green 25 of 25 under 96 spinners.#1895 — count dropped.
#1896 — reworded. You are right that
GetRequiredServicethrows at first render, so the failure mode is loud. The doc now says what the choice actually buys: no start-up dependency to lose, and both containers working with nothing registered. It also no longer claims the failure would be silent — only that loud is not the same as caught, since nothing in this repository resolves the application's container.#1897 — corrected in both places to "every test class that closes an overlay", and
PlaceDetailPanelTestsis now named as the counterexample that renders one without ever closing it. Verified: ten classes register the override, and they are exactly the ten athrow-in-the-fallback probe identifies as reaching the delay.#1898 — not changed, on the coordinator's ruling: the pattern stays and a ticket is being raised for a short ADR sanctioning
Corrected:the way ADR-0107 addedAnswered by, to regularise both this record and PR #200's.Baseline unchanged at 91.6; re-measured after these edits, still 2636/2876.
Verdict: mergeable
Re-reviewed at
ecde08b; review 325 superseded.The 150 ms lower bound is safe, measured rather than reasoned: 200 samples of
Task.Delay(200)idle and 200 more under 48 spinners on 24 cores gave minima of 199.10 ms and 199.23 ms, maxima 202 ms and 214 ms, and zero samples anywhere near 150. Earliest firing observed is ~0.8 ms ahead of the due time, so the 50 ms margin is roughly sixty times the largest early-fire, and contention only pushes elapsed time up. The residual effect of load is reduced mutation sensitivity — a no-op fallback masked by 150 ms of scheduling noise — never a false failure of the unmutated test, which is the right way round.Task.Delay(1)as the discriminating mutant checks out both ways: ~1 ms pends well past the next statement, so the oldIsCompletedcheck saw false and stayed green, while 1 ms is two orders below the new bound. The more valuable half is that awaiting the dispatch removes the old assertion's dependence on whetherDispatcher.InvokeAsynchad begun the work item at all.#1896 now states the mechanism correctly and draws the right conclusion from it. #1897's count matches: ten classes register the override, and
PlaceDetailPanelTestsis the only overlay-rendering class that never closes one.Corrected:left alone per the coordinator's ruling and task 268.ecde08b6d76dcb86a99dRebased onto
f931ef3(PR #200). Head is now6dcb86a; no conflicts, andcoverage-baseline.jsonwas untouched by #200 so nothing to resolve there.Baseline confirmed, not assumed: still 91.6. Six samples on the rebased head, read from
check-coverage-ratchet.cs's own output rather than by dividing line counts, allPlaceMark.WebUI: 91.6% (baseline 91.6%). Underlying figures identical to pre-rebase — 2636 of 2876 — with zero differing lines across the six.#200 did touch
MapMarker.cs, so this was worth checking rather than predicting: its edits there were doc comment and option-name changes that add no coverable lines, which is why the total is unchanged. Rebase verified as genuine —MapMarker.csis identical tomain,MarkerAccessibleNameJourneyTests.csis present, and this branch's diff againstmainis only its own 14 files.Full solution green on the rebased head.
Verdict: mergeable
Re-checked at
6dcb86a; review 326 superseded.The rebase is genuine and lossless.
merge_baseandbaseare bothf931ef3, and the branch's diff against it is the same 16 files with byte-identical per-file counts toecde08b— nothing gained, nothing dropped.MapMarker.csis not among them and reads asmaindoes, carrying task 262's correctedLabeldocumentation, with the record declaration itself untouched: the whole of #200's edit to that file sits inside an XML doc block, so it adds no coverable line and cannot move 2636/2876. Everything else #200 landed is present by construction, since the changed-file set excludes it.Spot-checked in place at the new head rather than assumed from the counts: the
Stopwatchbound and its 150 ms constant,CloseAsyncstill awaitingClosingDelay, the reworded lookup rationale, the baseline at 91.6, and the ten registering test classes.One correction to the hand-off note, no action needed: the branch's diff against
mainis 16 files, not 14 — same 16 as before the rebase.