Repository navigation
fix(frame): сделать владение scheduler атомарным - #157
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe shared frame loop now uses reservation identity, mutable subscriber entries, explicit scheduling lifecycle helpers, atomic cancellation, and immediate callback release. Tests cover scheduler edge cases, synchronized animation execution, rollback, cancellation, and garbage-collection retention. ChangesFrame loop lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Subscriber
participant FrameLoop
participant HostScheduler
participant Teardown
Subscriber->>FrameLoop: subscribe or update callback
FrameLoop->>HostScheduler: reserve and schedule frame
HostScheduler->>FrameLoop: deliver frame callback
FrameLoop->>Subscriber: execute phase callbacks
Subscriber->>FrameLoop: unsubscribe or cancelAll
FrameLoop->>Teardown: release callbacks and DOM references
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
a8dc1a8 to
957163f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frame/index.ts`:
- Around line 133-168: Update the native requestAnimationFrame invocation in
schedule so it preserves globalThis as the receiver, using a direct
globalThis.requestAnimationFrame call or an equivalent call(globalThis, fire).
Keep the injected requestFrame and fallback scheduling behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d5d1478-205f-4177-883b-9392dfc9d4dc
📒 Files selected for processing (6)
CHANGELOG.mdsrc/frame/index.tstest/animate-main-lane.test.tstest/fixtures/frame-retention-gc.probe.tstest/frame-loop.test.tstest/frame-retention-gc.test.ts
957163f to
703b891
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/frame-retention-gc.test.ts (1)
23-26: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding a timeout to the subprocess call.
If the probe script ever hangs (e.g., waiting on an unresolved promise or GC not triggering as expected), this test can block the run indefinitely with no bound.
♻️ Proposed fix
const output = execFileSync(process.execPath, ['--expose-gc', outfile], { cwd: ROOT, encoding: 'utf8', + timeout: 30_000, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/frame-retention-gc.test.ts` around lines 23 - 26, Update the execFileSync invocation in the subprocess test to include a finite timeout option, ensuring a hung probe terminates instead of blocking the test run indefinitely while preserving the existing cwd and encoding settings.test/frame-loop.test.ts (1)
296-871: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a
withFakeTimershelper to cut boilerplate.The
vi.useFakeTimers(); try { ... } finally { vi.useRealTimers(); }wrapper is repeated in ~15 tests across this block. A tiny helper would remove the duplication without risking unrelated tests (avoids reaching for a globalbeforeEach/afterEach, which could break any test elsewhere in the file that relies on real timers/microtasks).♻️ Suggested helper
function withFakeTimers(run: () => void): void { vi.useFakeTimers(); try { run(); } finally { vi.useRealTimers(); } }Then each test body becomes
it('...', () => withFakeTimers(() => { ... }));.Traced the scheduling/tick/rollback logic in this block against
createFrameLoop(src/frame/index.ts:71-230) for several representative cases (synchronous double-callback collapsing, handle=0 fallback demotion, resubscribe-during-tick isolation, cancelAll atomicity, stale-host-after-cancelAll) — all assertions match the given source semantics; no functional issues found.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/frame-loop.test.ts` around lines 296 - 871, Extract a local withFakeTimers helper in the test file that wraps vi.useFakeTimers(), executes a callback, and always restores real timers in finally. Refactor the repeated fake-timer tests in this block to invoke their bodies through this helper, preserving each test’s existing assertions and avoiding global timer hooks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/frame-loop.test.ts`:
- Around line 296-871: Extract a local withFakeTimers helper in the test file
that wraps vi.useFakeTimers(), executes a callback, and always restores real
timers in finally. Refactor the repeated fake-timer tests in this block to
invoke their bodies through this helper, preserving each test’s existing
assertions and avoiding global timer hooks.
In `@test/frame-retention-gc.test.ts`:
- Around line 23-26: Update the execFileSync invocation in the subprocess test
to include a finite timeout option, ensuring a hung probe terminates instead of
blocking the test run indefinitely while preserving the existing cwd and
encoding settings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a4484a2-db40-4124-b71a-848c9bbddf16
📒 Files selected for processing (6)
CHANGELOG.mdsrc/frame/index.tstest/animate-main-lane.test.tstest/fixtures/frame-retention-gc.probe.tstest/frame-loop.test.tstest/frame-retention-gc.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- CHANGELOG.md
- test/fixtures/frame-retention-gc.probe.ts
- src/frame/index.ts
- test/animate-main-lane.test.ts
|
CodeRabbit follow-up: добавлен жёсткий |
Пользовательский результат
Общий frame-loop больше не рекурсирует, не вклинивает вложенный кадр в незавершённую транзакцию и не удерживает callback/DOM после terminal-сценариев даже при нарушающем контракт host-clock. Обычный путь сохраняет один native rAF на кадр для всех потребителей.
Контракт
Порядок фаз
read → update → render, exact-once, немедленныйoff, ленивый start/stop и публичные типы не меняются. СинхронныйrequestFrameпереводится через отслеживаемый асинхронный trampoline. Native rAF вызывается с корректнымglobalThisreceiver, а injected scheduler остаётся receiver-free и не доверяет подменённому.call.cancelAll()атомарно завершает старых владельцев до исполнения нового цикла.Доказательство
9ea8a9d: frame/hostile 109/109--expose-gcдоказывает освобождение rollback/off/once callbackАрхитектура
liveдаёт O(1) idle-решение; один in-place compactor сохраняет фазовые границыoff,onceи rollback сразу обнуляют callback/teardown ссылкиПроизводительность
Структурный ceiling-гейт сохраняет один rAF/кадр для full, mini и cross-entry; массовый animate-path нейтрален. Независимый AB/BA-аудит Node 24 удалил non-Pareto tuple: финальный мономорфный object-shape быстрее tuple примерно на 19,8% в recurring N=1000 и на 10% в once-churn. Абсолютная correctness-цена terminal-путей остаётся микросекундной на 1000 владельцев и меньше 0,2% бюджета кадра 240 Гц.
Размер относительно merged
main#158:frame: 640 → 739 B gzip;animate: 11 844 → 11 943 B gzip, запас 57 B;animate/mini: 4 870 → 4 948 B gzip, запас 52 B;Риски и не-цели
Документация и выпуск
CHANGELOG.mdописывает действующий новый контрактГейты
pnpm check:staticpnpm buildpnpm test: 3 480/3 480pnpm sizepnpm test:browser:all: 99/99pnpm pack:smokepnpm pack:compatpnpm bench:ceiling: structural PASS703b891Связано с #152