test(root): compare the running effect against what the stylesheet declares - #867
Conversation
…clares Three refusals already exist for effects whose real timing is not in any longhand: a scripted animation, a declared one whose playback rate was changed, and a non-document timeline. `updateTiming` is a fourth route to the same place and it defeats all three — the object stays a `CSSAnimation`, the playback rate stays 1, and every computed longhand still describes the stylesheet. So this compares rather than enumerating. `activeDuration` already folds duration by iterations and any override, so no effect has to be matched back to the rule that produced it, and the comparison covers routes nobody has found yet. Only the under-reporting direction is refused. A live span shorter than the declaration makes the probe wait longer than the edge moves, which is safe. The live side counts only geometry-moving effects, because the declared side does. Counting every owned effect compares two populations rather than two numbers, and the canvas has a real background transition that moves no edge.
|
@codex please review this PR |
|
Warning Review limit reached
Next review available in: 16 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
@nextlyhq/adapter-drizzle
@nextlyhq/adapter-mysql
@nextlyhq/adapter-postgres
@nextlyhq/adapter-sqlite
@nextlyhq/admin
@nextlyhq/admin-css
@nextlyhq/blocks-engine
@nextlyhq/blocks-react
@nextlyhq/builder
create-nextly-app
nextly
@nextlyhq/plugin-form-builder
@nextlyhq/plugin-page-builder
@nextlyhq/plugin-sdk
@nextlyhq/plugin-seo
@nextlyhq/storage-s3
@nextlyhq/storage-uploadthing
@nextlyhq/storage-vercel-blob
@nextlyhq/ui
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30e80efa1d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ect is Two holes in the comparison, both of which let it report the declared span while the edge keeps moving. The classification had two states where the declared path has three. A property on neither list can move an edge — `border-bottom-width` does — so reading "not geometry" as "does not move geometry" dropped the effect from the comparison entirely, and retiming such an effect was invisible. Unknown now refuses through the same channel the declared path uses. And the span is measured from where the effect currently is. Moving a schedule rather than a duration leaves `activeDuration` and `delay` untouched, so their sum equals the declaration while an animation rewound to a negative current time keeps moving for a second longer. `endTime` minus `currentTime` covers both, and can only come out shorter as an effect progresses, which is the safe direction.
|
@codex please review this PR |
|
@nextly-bot review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41ba5ccab8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Claude finished @mobeenabdullah's task in 36m 12s —— View job Review round 1 — not posted (superseded)
The review was written and submitted against What the withheld review would have said, for the record against No new findings (0 P0/P1/P2/P3). I attacked the PR's stated guarantee along four routes Codex did not name, and each is covered:
The two resolved Codex threads are genuinely fixed at head, and their controls are load-bearing: the three-state Four Codex threads remain open and unanswered (paused effects, empty-but-readable keyframes, positive Process checks passed: test-only diff (1 file, +308), |
…nimates Four holes, two that let the comparison under-report and two where it refused a canvas that was fine. A PAUSED effect leaves the playback rate at 1 while its clock stops, so every span computed from it is a remainder that never shrinks and the edge moves again whenever something resumes it. Refused, like the rate-zero case it reaches by a different API. A transition's `transitionProperty` names what CREATED it, not what it animates now. `setKeyframes` can replace that, and a paint transition rewritten to move height was classified safe while the declared path skipped the paint longhand — so nothing charged it at all. The keyframes are read alongside the name. An empty keyframe list is a SUCCESSFUL read proving the effect moves nothing, so it is no longer treated as unreadable and refused. And `endDelay` is a post-active tail rather than movement, so the span is measured over the active interval instead of to `endTime`.
|
@codex please review this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a6b2e4b6c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@nextly-bot review |
|
Claude finished @mobeenabdullah's task in 35m 12s —— View job Review posted — round 1The review round ran to completion and was posted against
Findings that survived:
Not executed: the Playwright suite — |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nextly Review Bot: round 1 - findings on the live-effect comparison
Verdict: The live-vs-declared comparison is the right shape and the six controls are genuinely separating, but two states still let an owned effect out-run its declaration without the comparison seeing it (playState: "idle" with a delay, and a negative endDelay that truncates the active interval). Both are the same direction the PR says it refuses, so they belong in this PR rather than a follow-up.
New findings: 1 P1, 1 P2, 1 P3 (inline below)
Prior rounds: none by this bot (no pr-review-agent review exists on this PR). The six resolved Codex threads were each re-verified at head: unclassified routing (three-state classify), the currentTime subtraction, the paused clause, the empty-keyframes read, the active-interval-not-endTime span, and the keyframes-read-for-both-kinds union are all present and each is backed by a control. The three unresolved Codex threads (negative endDelay, zero-iteration fill, pause-before-classify) overlap the findings below; see the duplicate notes on each.
Not inline-anchorable: none
Pre-existing (not this PR): none noted. The e2e AGENTS.md files are absent from this checkout (git show HEAD:e2e/AGENTS.md and .claude/AGENTS.md both fail), so package-local rules could not be read.
Checked:
- Attacked the stated guarantee ("refuse when live > declared") against every Web Animations state reachable through
updateTiming/currentTime/pauseon an ownedCSSAnimation; two bypasses survived. - Re-derived the live span by hand for each of the seven new controls and confirmed each assertion is the separating property, not a precondition (AGENTS.md L194-L204).
- Verified the three prior-round fixes against the head code path, not the resolution click.
- Did not execute the suite:
node_modulesis absent in this runner and installing the workspace plus building the playground on :3100 was out of scope for this run. Nothing below depends on a run; each is argued from the API contract in the code. - Changeset: correctly omitted for a test-only PR (AGENTS.md L306). Title
test(root): ...is conventional,rootis in thepr-title.ymlscope allowlist, subject is lowercase imperative with no trailing period. No AI attribution in the three commits or the PR body.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02e3248ccb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return [ | ||
| ...declared, |
There was a problem hiding this comment.
When a geometry CSSTransition is replaced with paint-only or empty keyframes via setKeyframes() and then given a longer duration, this unconditional inclusion of declared still classifies the effect as geometry-moving. The live/declaration comparison therefore refuses even though the readable effect can no longer move an edge; this is inconsistent with the empty-animation handling immediately above. Use the original transition property only as the unreadable fallback, and classify successfully read transitions from their actual keyframes.
AGENTS.md reference: AGENTS.md:L198-L202
Useful? React with 👍 / 👎.
|
@codex please review this PR |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
test(root): recover the clock-bound commit stranded by #867
1 participant
Closes the last known UNDER-reporter in the canvas settle guard — the direction that lets an allowance certify a wait shorter than the movement.
The gap
effect.updateTiming({ duration })retimes a declared animation and leaves every value this probe reads unchanged: the object is still aCSSAnimation,playbackRateis still 1, and every computed longhand still describes the stylesheet. All three existing refusals — scripted effect, changed playback rate, non-document timeline — are blind to it, and the probe charges the declaration while the edge travels longer.Why a comparison rather than a fourth special case
Three findings so far are one shape: the live effect disagrees with what the stylesheet says. Enumerating routes means finding the fourth in review and the fifth in production.
getComputedTiming().activeDurationalready folds duration × iterations and any override, so no effect has to be matched back to the rule that produced it. One comparison covers the routes nobody has found yet:Only the under-reporting direction is refused. A live span shorter than the declaration makes the probe wait longer than the edge moves, which is safe.
Two corrections found by running it, both worth reading
1. The live side must match the declared side's DOMAIN, not just its expression. My first version counted every owned effect and immediately failed four existing cases with "claims 100ms while its declaration reads 0ms" — the canvas's real
background .1stransition. That is a paint transition that moves no edge, and charging it reinstates exactly the ~100ms false floor #825 removed. The live side now counts only geometry-moving effects: a transition names its property, an animation's computed keyframes carry theirs, and an effect whose keyframes cannot be read is charged rather than skipped.2.
activeDuration === 0has two causes and they charge differently. Zero ITERATIONS runs nothing, so it holds the edge for nothing — not even its delay. Zero DURATION waits out its delay and then moves instantly, so the delay is the span.activeDurationalone cannot tell them apart; the declared path already separates them, so the live path reads the iteration count to agree. Caught bya zero-iteration animation charges neither duration nor DELAY, which failed on the first version.The control, and its failure
A
CSSAnimationretimed from a declared 20000ms to 60000ms. The control asserts the three readings that make it the case it claims to be —playbackRateis 1,animation-durationis still0.2s,animation-iteration-countis still100— and that the effect reports 60000ms. Without those the fixture could be refused by some other clause and prove nothing.Finite and still running, for the same reason the playback-rate control was rewritten from
infiniteto.2s 100: an endless animation is already refused for being endless, so an endless fixture passes with the comparison removed.Break verified. Comparison disabled → only the new control fails, with
Received promise resolved instead of rejected; the other 24 pass. Restored → 25 pass.Verification
check-types+lintgreen for@nextlyhq/e2e, turbo--force.One run was discarded rather than reported: a worker was
SIGKILLed and every case failed onECONNREFUSED :3100, which is a server that did not boot, not evidence.No changeset: test-only.