test(app): add runtime CLS source gate - #820
Merged
Merged
Conversation
Astro-Han
added
enhancement
github-actions
Bot
added
task
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a browser-injected Runtime CLS probe, Playwright lifecycle wrappers, unit tests for source classification/formatting, deterministic e2e gate specs for composer growth/shrink and dock close, and CI/npm script hooks to run the gate. ChangesRuntime CLS probe and e2e gate
Sequence DiagramsequenceDiagram
participant PlaywrightTest
participant Page as BrowserPage
participant Probe as window.__pawwork_runtime_cls_probe
participant PerfObs as PerformanceObserver
PlaywrightTest->>Page: addInitScript(init probe)
PlaywrightTest->>Probe: start(action, targetMessageID)
Probe->>PerfObs: subscribe(layout-shift)
Note over PerfObs,Probe: LayoutShift entries captured & classified
PlaywrightTest->>Page: perform UI interactions (composer/dock)
PlaywrightTest->>Probe: stop()
Probe-->>PlaywrightTest: RuntimeClsResult (entries, snapshot)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a performance monitoring framework for Cumulative Layout Shift (CLS) within E2E tests. It includes a browser-side probe that utilizes the PerformanceObserver API to detect and classify layout shifts, a new suite of Playwright tests targeting UI interactions like composer resizing and dock closure, and unit tests for the classification logic. The reviewer feedback focuses on improving the robustness of the probe by ensuring that initialization errors are not swallowed and that internal state is properly reset after each measurement to prevent data leakage between test iterations.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/app/e2e/perf/runtime-cls-gate.spec.ts (2)
24-39: 💤 Low valueRename test constants to SCREAMING_SNAKE_CASE.
runtimeClsSeedTurns,runtimeClsMinimumRows,runtimeClsMaximumMountedMessages,composerGrowthText, andquestiondon't match the repo naming rule for constants in*.spec.tsfiles. As per coding guidelines,packages/app/e2e/**/*.spec.ts:Use SCREAMING_SNAKE_CASE for constants in tests.🤖 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 `@packages/app/e2e/perf/runtime-cls-gate.spec.ts` around lines 24 - 39, Rename the test constants to SCREAMING_SNAKE_CASE: change runtimeClsSeedTurns -> RUNTIME_CLS_SEED_TURNS, runtimeClsMinimumRows -> RUNTIME_CLS_MINIMUM_ROWS, runtimeClsMaximumMountedMessages -> RUNTIME_CLS_MAXIMUM_MOUNTED_MESSAGES, composerGrowthText -> COMPOSER_GROWTH_TEXT, and question -> QUESTION; update all references in this spec (e.g., where RUNTIME_CLS_SEED_TURNS, RUNTIME_CLS_MINIMUM_ROWS, RUNTIME_CLS_MAXIMUM_MOUNTED_MESSAGES, COMPOSER_GROWTH_TEXT, and QUESTION are used) so imports/uses remain consistent and run the tests to ensure no breakage.
3-3: ⚡ Quick winDon't mix fixture tracking with manual child-session deletion.
Once Line 318 registers
child.idwithproject.trackSession(...), thefinallyblock adds a second cleanup path for the same resource. That makes teardown order-dependent for no gain; keep the fixture-managed path and drop the explicitcleanupSession(...). As per coding guidelines,packages/app/e2e/**/*.spec.ts:Call project.trackSession(sessionID, directory?) and project.trackDirectory(directory) for any resources created outside the fixture so teardown can clean them up.Suggested cleanup
-import { cleanupSession, seedSessionQuestion, withSession } from "../actions" +import { seedSessionQuestion, withSession } from "../actions" ... - try { - await test.step("seed child question dock outside the measured window", async () => { - await llm.toolMatch(inputMatch({ questions: question }), "question", { questions: question }) - await seedSessionQuestion(project.sdk, { sessionID: child.id, questions: question }) - }) - const targetMessageID = - await test.step("reveal a long visible parent timeline window with the dock open", async () => { - const target = await prepareRuntimeClsWindow(page, project, session.id) - await expect(dock).toBeVisible({ timeout: 30_000 }) - await settleFrames(page, 6) - return target - }) - - const result = await test.step("close the child question dock under the runtime CLS probe", async () => { - await startRuntimeClsProbe(page, "question-dock-close", { targetMessageID }) - await dock.getByRole("radio", { name: /Continue/i }).click() - await dock.getByRole("button", { name: /submit/i }).click() - await expect(dock).toHaveCount(0) - await expect(page.locator(promptSelector).first()).toBeVisible() - await settleFrames(page, 6) - return await stopRuntimeClsProbe(page) - }) - - await assertNoPrimaryRuntimeClsFailures(result) - } finally { - await cleanupSession({ sdk: project.sdk, sessionID: child.id }) - } + await test.step("seed child question dock outside the measured window", async () => { + await llm.toolMatch(inputMatch({ questions: question }), "question", { questions: question }) + await seedSessionQuestion(project.sdk, { sessionID: child.id, questions: question }) + }) + const targetMessageID = + await test.step("reveal a long visible parent timeline window with the dock open", async () => { + const target = await prepareRuntimeClsWindow(page, project, session.id) + await expect(dock).toBeVisible({ timeout: 30_000 }) + await settleFrames(page, 6) + return target + }) + + const result = await test.step("close the child question dock under the runtime CLS probe", async () => { + await startRuntimeClsProbe(page, "question-dock-close", { targetMessageID }) + await dock.getByRole("radio", { name: /Continue/i }).click() + await dock.getByRole("button", { name: /submit/i }).click() + await expect(dock).toHaveCount(0) + await expect(page.locator(promptSelector).first()).toBeVisible() + await settleFrames(page, 6) + return await stopRuntimeClsProbe(page) + }) + + await assertNoPrimaryRuntimeClsFailures(result)Also applies to: 318-318, 320-346
🤖 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 `@packages/app/e2e/perf/runtime-cls-gate.spec.ts` at line 3, The test registers child.id with project.trackSession(...) but then also calls cleanupSession(...) in the finally block—remove the explicit cleanupSession(...) call and rely on project.trackSession(sessionID, directory?) to manage teardown; locate the finally block around the child session creation (where child.id is registered) and delete the manual cleanupSession(...) invocation so the fixture-managed teardown is the sole cleanup path.
🤖 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 `@packages/app/e2e/perf/runtime-cls-probe.ts`:
- Around line 521-546: The PerformanceObserver setup currently swallows errors;
change the block around PerformanceObserver/observer.observe so that any
exception sets a persistent probe error/unsupported flag (e.g., set a
module-level boolean or an Error stored on this probe) instead of silently
catching, and ensure start() and stop() check that flag and throw a descriptive
error when the observer failed to initialize (include context like "layout-shift
observer failed to start"). Also preserve the existing
entries/startedAt/maxEntries logic but mark the probe as unusable when
observer.observe throws so CI fails closed rather than silently recording zero
shifts.
- Around line 304-305: installRuntimeClsProbe currently only calls
page.addInitScript which affects future navigations/frames, leaving an
already-loaded document unpatched and causing startRuntimeClsProbe to fail;
modify installRuntimeClsProbe to also inject the same probe into the current
document (for example by running the same probe function body via page.evaluate
or by using page.addScriptTag with the probe content) so the probe is present
immediately and in future navigations. Ensure the injected logic matches the
existing addInitScript payload and reference the functions
installRuntimeClsProbe and startRuntimeClsProbe when applying the change.
---
Nitpick comments:
In `@packages/app/e2e/perf/runtime-cls-gate.spec.ts`:
- Around line 24-39: Rename the test constants to SCREAMING_SNAKE_CASE: change
runtimeClsSeedTurns -> RUNTIME_CLS_SEED_TURNS, runtimeClsMinimumRows ->
RUNTIME_CLS_MINIMUM_ROWS, runtimeClsMaximumMountedMessages ->
RUNTIME_CLS_MAXIMUM_MOUNTED_MESSAGES, composerGrowthText ->
COMPOSER_GROWTH_TEXT, and question -> QUESTION; update all references in this
spec (e.g., where RUNTIME_CLS_SEED_TURNS, RUNTIME_CLS_MINIMUM_ROWS,
RUNTIME_CLS_MAXIMUM_MOUNTED_MESSAGES, COMPOSER_GROWTH_TEXT, and QUESTION are
used) so imports/uses remain consistent and run the tests to ensure no breakage.
- Line 3: The test registers child.id with project.trackSession(...) but then
also calls cleanupSession(...) in the finally block—remove the explicit
cleanupSession(...) call and rely on project.trackSession(sessionID, directory?)
to manage teardown; locate the finally block around the child session creation
(where child.id is registered) and delete the manual cleanupSession(...)
invocation so the fixture-managed teardown is the sole cleanup path.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 86e1e673-de88-4788-a4fd-48ec5ee209e5
📒 Files selected for processing (5)
.github/workflows/perf-probe-baseline.ymlpackages/app/e2e/perf/runtime-cls-gate.spec.tspackages/app/e2e/perf/runtime-cls-probe.tspackages/app/e2e/perf/runtime-cls-probe.unit.tspackages/app/package.json
Perf delta summaryComparator: pass
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/app/e2e/perf/runtime-cls-gate.spec.ts`:
- Around line 277-305: The test currently never emits a layout-shift while the
probe is active, so add a synthetic entry emission between
startRuntimeClsProbe(...) and the first stopRuntimeClsProbe(...) to prove
active-window capture: after await startRuntimeClsProbe(page, "first-window", {
targetMessageID: "msg-1" }) call, invoke the page.evaluate block that calls
window.__emitRuntimeClsEntry with a layout-shift entry (targeting the same node)
and then call stopRuntimeClsProbe to assert that the first stop returned an
entries array containing that emitted entry; keep the existing post-stop
evaluation that emits another entry and asserts repeatedStop?.entries is empty
to verify post-stop ignores.
In `@packages/app/e2e/perf/runtime-cls-probe.ts`:
- Around line 518-523: The direct-primary branches that return for
element.matches("[data-message-id]") and
element.matches('[data-component="session-turn"]') need the same visibility
gating used elsewhere: require primaryAncestor?.visibleBefore &&
primaryAncestor?.visibleAfter before classifying as "primary-message-wrapper" or
"primary-turn" so off-screen wrappers/turns aren't treated as visible timeline
content; update the runtime classifier (the ifs using element.matches(...)) and
mirror the identical visibility check in the exported classifier above so
collectRuntimeClsFailures() and the unit-helper path remain aligned.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 240166cb-2cee-4184-985d-6d5f48c23923
📒 Files selected for processing (2)
packages/app/e2e/perf/runtime-cls-gate.spec.tspackages/app/e2e/perf/runtime-cls-probe.ts
Owner
Author
|
Addressed CodeRabbit's two nitpick comments in 261b767:
Follow-up CodeRabbit inline comments were addressed in 13e94f8:
Verification run locally:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a head-only runtime CLS source gate for session composer and question dock height changes.
Why
#814 needs a controlled regression gate for runtime timeline shifts during composer / prompt dock resizing. This PR observes real
LayoutShiftentries during bounded interaction windows and fails on large visible timeline primary-source movement, including nestedprimary-turn-descendantsources called out in design review.Related Issue
Closes #814.
Follow-up: #818 tracks deterministic question dock open/growth coverage. This PR covers composer growth, composer shrink, and question dock close/shrink; open/growth is deferred because current deterministic question seeding would mix dock opening with tool-message hydration.
Human Review Status
Pending
Review Focus
primary-turn-descendantclassification: assistant/message-part descendants inside visible primary ancestors must not be swallowed as residual diagnostics.Risk Notes
How To Verify
Screenshots or Recordings
Not required; this PR adds runtime/perf test coverage and CI wiring only, with no visible UI or copy changes.
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Summary by CodeRabbit
Tests
Chores