Animate the board's columns from JS, so the entrance cannot play twice #32
No reviewers
Labels
No labels
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Grandiras/Ponente!32
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "deterministic-board-entrance"
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?
Yes, and that's what this does. Every previous attempt was a timer trying to out-guess DOM churn, which is why each one produced a new symptom instead of a fix.
Why the timers kept losing
The board is built twice on every load — the prerender paints columns, the circuit's first render replaces them with
Board.razor's spinner, then the data builds them again (production: 82 ms / 311 ms / 317 ms). A CSS animation keyed to element creation fires on whichever sets happen to exist.What these were racing is the framework's own render schedule. No amount of tuning wins that.
What this does instead
There is no CSS animation on the columns any more.
Board.razorcallsponenteEntrance.playBoard(_columns.Count)from the render where its data has actually arrived — the same signal it already trusts to decide when its drag-and-drop interop can safely import — and JS animates exactly the elements present at that moment, once, via the Web Animations API.Anything created afterwards cannot animate, because there is no rule left to fire on it. That's the deterministic part: correctness no longer depends on when the rebuild happens.
One subtlety cost me an intermittent "animates nothing at all" while building this, and is worth stating:
Board.razor's_columns is not nullis component state, not the DOM.OnInitializedAsync's continuation can land between the spinner render and itsOnAfterRender, so the call sometimes arrives while the spinner is still on screen and querying for columns finds none. It now passes the count it rendered, and JS waits for that many columns — a wait on the thing being animated rather than on a clock.Keyframe values live in
app.cssas custom properties (--entrance-duration,--entrance-lead,--entrance-stagger,--entrance-rise) and are read back off:root, so the stylesheet stays the source of truth instead of the timings being duplicated into JS.Verification
The assertion is now the strongest available and identical at every timing: as many animations started as there are columns, and all of them finished. A cancelled animation is the flicker; an extra batch is the double.
The safety reveal is a guard for a board that never renders at all (dead circuit, a throw in
ReloadAsync), not a timing heuristic; past it the entrance is given up rather than played over content already on screen.Three consecutive full runs of the file, no flakes.
dotnet build -c Releaseclean,dotnet test46/46, Playwright 77/77.CLAUDE.md now says plainly that the columns must not be put back on the CSS gate, and records that delaying
blazor.web.jsto reproduce this gives a false negative — the trap that cost two releases.🤖 Reviewing this PR — follow along in the action run; the reply will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the task runs.
💬 Verdict: comment — notes only, nothing blocking.
This is a well-reasoned fix and matches the failure mode described in the PR: gating the columns' entrance on a JS call driven by
Board.razor's own render, rather than a CSS rule keyed to element creation, removes the race the previous three attempts kept losing. I traced the flicker concern the PR itself raises (reveal()synchronously removing the CSS-hide rule beforeanimateColumns()applies the WAAPIfill: backwardsstate) and it holds up:whenRenderedalways waits at least onerequestAnimationFramebefore checking the DOM, so the animation's backward-filled opacity-0 state is applied in the same frame the hidden-rule's style invalidation would otherwise have painted, avoiding a visible flash. Theplayed/pushState reset interaction between overlappingwhenRenderedpolls on rapid navigation also checks out — a stale loop bails viaif (!played) returnrather than animating stale state. Tests were updated consistently with the new WAAPI-based mechanism and the newUNBLOCK_MSsafety window.One real edge case survives, noted below: an empty board (zero columns) leaves the JS in an unbounded polling loop.
⏱ 2m 39s · 10 turns · tokens: 48.0k in / 13.0k out (+546.8k cache-read)
📜 Full session transcript
@ -124,0 +172,4 @@var look = function () {if (!played) return;var columns = document.querySelectorAll('.board > .column');if (columns.length >= expected) { then(columns); return; }whenRendered(expected || 1, animateColumns): when the board has zero columns — a real, reachable state (Board.razorhas a dedicated_columns.Count == 0empty-state branch, e.g. right after an admin deletes the last column) —Board.razorcallsplayBoard(0).0 || 1coercesexpectedto1, andwhenRendered'slookloop then pollsdocument.querySelectorAll('.board > .column')viarequestAnimationFrameforever, since no.columnelement will ever appear while the empty state is showing.reveal()has already run so nothing stays visually hidden, but the tab is left spinning an unbounded per-frame busy loop for the rest of the page's lifetime (until a route change resetsplayed). Worth guarding with something likeif (!expected) return;before callingwhenRendered, so a genuinely empty board doesn't leave JS polling forever.Good catch — fixed in the latest push, and it was worse than "the tab spins":
reveal()runs before the wait, so nothing would have been visibly wrong. It would only have shown up as battery drain.playBoard(0)now returns immediately, and the wait is bounded for the general case of columns that never arrive (a throw in the render). Giving up is safe there for the same reason it's safe in the empty case: the board is already visible by then, so the cost is a missing animation rather than hidden content.On the test: my first attempt reached the empty state by deleting the fixture's columns through the admin UI. That was wrong twice over — the selectors matched nothing, so it asserted on a board that still had three columns and passed vacuously; and had it worked, it would have left the shared test database with no columns for every later spec, since teardown restores rows it created, not ones a test removed. It now calls both guards directly on a page with no board.
Verified against the unguarded version: 43 frames scheduled where fewer than 10 are expected.
@claude /review
🤖 Reviewing this PR — follow along in the action run; the reply will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the task runs.
🛑 Verdict: request changes — blocking issues below.
The empty-board polling issue I flagged last pass is fixed (
if (!expected) return;beforewhenRendered), and the newboard-entrance.spec.jstest explicitly pins both the empty-board and never-arrives cases via the frame-count guard. The WAAPI-driven column entrance itself is sound:playBoardis called from the render where_columnsdata has actually landed, animates exactly the elements present once, and there's no CSS rule left to fire on later churn — this genuinely removes the class of race the last three attempts kept losing.However, this diff also rewrites the unrelated auth-card fallback in the same file's first IIFE, and that change looks like a real regression that isn't mentioned anywhere in the PR description. See inline comment.
⏱ 1m 5s · 3 turns · tokens: 35.6k in / 5.4k out (+151.7k cache-read)
📜 Full session transcript
@ -105,2 +77,2 @@// admin page. Routes.razor cannot help — those navigations reuse the// circuit, so its first render happened long ago.// Switching between Sign in and Create account is the one replay the design// does want. Routes.razor cannot help here — that navigation reuses theThis changes the login-card fallback from
reveal()toplay()(and bumpsCIRCUIT_FALLBACK_MSfrom 900ms to 3000ms), which is out of scope for a PR titled about the board columns and isn't mentioned in the description. It looks like a reintroduction of the exact bug class this PR is fixing for columns, just for the card:If the circuit takes longer than 3000ms to report (slow, not dead), this timer now calls
play()on the prerendered card — settingdata-auth="playing", which the CSS rulehtml[data-auth="playing"] .login-card > *keys off element creation. When Blazor's circuit eventually connects and discards/recreates the prerendered card's children shortly after, those newly-created nodes match the still-playingselector and animate again — a second cascade on content that already finished (or was mid-) animating. That's the same "CSS animation restarts whenever its element is created" hazard CLAUDE.md documents as the original card bug, and the whole reasonentrance.jspreviously calledreveal()(never-animate) rather thanplay()on this fallback.Previously
spent/reveal()made this safe by ensuring a late arrival could never animate content already on screen; that guard is now gone from the fallback path entirely. CLAUDE.md's description of this fallback ("if the circuit has not reported within ~900ms it reveals rather than plays, and marks the entrance spent so a late arrival cannot animate content already on screen") was also left unchanged, so the docs are now inconsistent with the code.auth-entrance.spec.jshas no test past ~1500ms of circuit delay for the card, so this path isn't covered.If this fallback change is intentional, it needs its own justification (why is animating the prerendered card now safe?) and a CLAUDE.md update; if it's accidental scope creep from refactoring the shared file, it should be reverted to
fallback = setTimeout(reveal, CIRCUIT_FALLBACK_MS)withspentrestored.Correct on every point, and it was my error rather than a deliberate change. I started the board work by restoring the card's state machine from
v2.0.2as a clean base, which silently took the fallback back to v2.0.2 behaviour as well:play()at 3000 ms instead ofreveal()at 900 ms, withspentgone.So yes — that reintroduces, for the card, precisely the bug class this PR removes for the columns. A circuit that is slow rather than dead trips the timer, sets
data-auth="playing"on the prerendered card, and then Blazor connects and recreates the card's children, which the CSS keys off element creation. Second cascade on content already on screen.Telling detail: CLAUDE.md still described the v2.0.4 behaviour throughout. The docs were right and the code was wrong, which is the wrong way round for that to be caught by anything but review.
Fixed in the latest push:
reveal()andspentare back, and a route change now clearsspent— a new arrival says nothing about whether the previous entrance was spent. Also added the coverage that was missing: the card at a 2500 ms circuit delay must be visible early and must not animate when the circuit finally lands.auth-entrance.spec.jshad nothing past ~1500 ms, which is exactly why this slipped through.79 passing, build clean.
@claude /review
🤖 Reviewing this PR — follow along in the action run; the reply will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the task runs.
🤖 Reviewing this PR — follow along in the action run; the reply will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the task runs.
✅ Verdict: approve — no blocking issues found.
Both blocking issues from my previous passes are resolved. The empty-board infinite-poll bug is fixed with
if (!expected) return;beforewhenRendered, and now has a dedicated test (waiting for columns cannot spin forever). The out-of-scope regression I flagged last round — the auth-card fallback switching fromreveal()toplay()withCIRCUIT_FALLBACK_MSbumped to 3000ms, which would have replayed the card entrance on newly-created nodes from a late-connecting circuit — has been reverted back toreveal()at 900ms, matching the documented behavior.The core mechanism is sound:
Board.razorcallsponenteEntrance.playBoard(_columns.Count)exactly once per circuit (guarded by_entrancePlayed, which is never reset by the mutation-triggeredReloadAsynccalls, only by client-side route changes in JS), animates only the columns present via WAAPI, and there's no CSS rule left to fire on later churn — this genuinely removes the race the prior three attempts kept losing. Thereveal()-before-animate ordering avoids the flicker concern I traced in an earlier pass (the WAAPIfill: backwardsstate is applied within the same rendering frame as the CSS-hide removal), theplayed/shownAlreadyinterplay correctly skips animating content that theUNBLOCK_MSsafety guard already revealed, and the boundedRENDER_WAIT_MSpoll can't spin forever. Tests updated consistently (started/finished animation counts replace the old animationstart-count assertions) and cover 0/250/700/1500ms circuit delays plus the >UNBLOCK_MS case.One minor, non-blocking note below: this diff also silently drops the
popstatelistener that previously let browser back/forward navigation replay the card and board entrances — worth a mention in the PR or a quick follow-up, but not a functional break (worst case is a missing animation, and it isn't tested either way).⏱ 2m 33s · 12 turns · tokens: 47.7k in / 11.6k out (+790.7k cache-read)
📜 Full session transcript
@ -124,0 +184,4 @@// Board.razor tells us how many columns it just rendered. That matters// because its own guard ("_columns is not null") is component state, not the// DOM: OnInitializedAsync's continuation can land between the spinner renderThe old file's shared
routeChangedwas wired to both the patchedhistory.pushState/replaceStateandwindow.addEventListener('popstate', routeChanged), so browser back/forward navigation (e.g. admin page → back to the board, or between login tabs) replayed the relevant entrance. This split rewrite dropspopstateentirely: the card's replay is now gated on a click on.auth-tab, and the board's replay is gated only on the patchedpushState/replaceState. Blazor's router does not re-invokepushState/replaceStatefor a native back/forward navigation — the browser already owns the URL and firespopstatedirectly — so returning to the board or a login tab via the browser back/forward buttons no longer resetsplayed/spent, and the entrance silently doesn't replay.Not mentioned in the PR description and not covered by a test (
board-entrance.spec.js's "returning to the board animates it again" usespage.goto, a full reload, nothistory.back()). The worst case is a missing animation rather than a functional bug, so this doesn't block merging, but it's the same kind of silent scope-adjacent change to this shared file that the last review round had to catch — worth confirming it's intentional.✅ Verdict: approve — no blocking issues found.
The blocking issue from my last pass — the auth-card fallback silently switching from
reveal()toplay()with nospentguard, which would have re-animated Blazor-recreated card children on a slow-but-live circuit — is fixed. It's back toreveal()on theCIRCUIT_FALLBACK_MStimer,spentis restored, and CLAUDE.md's description of that fallback now matches the code.The empty-board polling issue from my first pass was already fixed in the previous revision and remains fixed here (
if (!expected) return;beforewhenRendered).The core mechanism is sound:
Board.razorcallsponenteEntrance.playBoard(_columns.Count)from the render where its data has actually landed (the same render that gates the drag-and-drop interop import), JS animates exactly the elements present via WAAPI once, and there is no CSS rule left keyed to element creation for anything created afterwards to replay.reveal()removing the CSS-hide happens synchronously insideplayBoard, butwhenRenderedalways waits at least onerequestAnimationFramebefore reading the DOM, so the WAAPIfill: 'backwards'state is applied within the same frame the un-hide would otherwise paint — no flash.played/shownAlreadycorrectly gate against the empty-state, the never-arrives case (bounded byRENDER_WAIT_MS), the safetyUNBLOCK_MSreveal, and stale polls after a route-change reset (if (!played) returninsidelook). Tests were updated consistently with the WAAPI-based mechanism (started/finishedcounts instead ofanimationstart), and the newboard-entrance.spec.jscases cover the safety-reveal path and the empty/never-arrives polling bounds directly rather than by deleting fixture data.One minor, non-blocking observation below: this revision also drops the shared
popstatelistener entirely fromauth-enter.js(both the card's and — newly — the board's re-entrance are now driven only by patchedpushState/replaceStateplus, for the card, an explicit.auth-tabclick listener). That's a small behavioral change not mentioned in the PR description, but it degrades gracefully (a missed replay, not broken content) and is unlikely to matter in practice since the primary path is clicking the tab control, not the browser back button.⏱ 2m 39s · 13 turns · tokens: 21.3k in / 12.3k out (+927.3k cache-read)
📜 Full session transcript
@ -130,4 +283,3 @@return result;};});window.addEventListener('popstate', routeChanged);This PR removes the shared
window.addEventListener('popstate', routeChanged)entirely (visible as a deletion in the diff, with no replacement in either the card's or the new board IIFE). Browser back/forward navigation between the sign-in/create-account tabs (or, in principle, back to the board) no longer replays the entrance — onlyhistory.pushState/replaceStateand, for the card, clicks on.auth-tabare watched now. Not called out in the PR description, and worth a one-line mention if intentional, but low severity: the fallback is a missing animation rather than any visible breakage, and the primary interaction path (clicking the tab) is unaffected.