Fix test infrastructure failures exposed after removing retries

[Copilot]

Removing retries exposed pre-existing test failures that were previously masked. All are infrastructure/config issues, not test logic bugs.

Changes

app/api/debug/reset/route.ts

The Next.js route guarded by NODE_ENV !== 'development' was returning 403 on all production test runs, even when ALLOW_DEBUG_RESET=true. Aligned the guard with the Express handler in server.ts:

// Before
if (process.env.NODE_ENV !== 'development') { return 403 }

// After
if (env.NODE_ENV === 'production' && !env.ALLOW_DEBUG_RESET) { return 403 }

next.config.js

NEXT_PUBLIC_TESTING: process.env.TESTING in the env block takes precedence over directly-set env vars. When TESTING is absent at build time (reused server, pre-built binary), this bakes NEXT_PUBLIC_TESTING as undefined — causing isTestEnvironment() to return false and window.__TEST_CONTROLS__ to never be populated.

// Before: overrides NEXT_PUBLIC_TESTING=true with undefined when TESTING is unset
NEXT_PUBLIC_TESTING: process.env.TESTING

// After: falls back to the directly-set env var
NEXT_PUBLIC_TESTING: process.env.TESTING ?? process.env.NEXT_PUBLIC_TESTING

tests/playwright/lib/visual.ts

Introduced waitForVRTReady(page, targetWidth?) — a deterministic readiness primitive replacing arbitrary waitForTimeout calls and ad-hoc waitForFunction boilerplate in spec files. Standardized screenshot thresholds to maxDiffPixelRatio: 0.1 / threshold: 0.2. Added locator-safe scroll guard (if (!isLocator)) so window.scrollTo(0, 0) is never called on Locator targets (prevents MUI Popover detachment). Removed the over-engineered expect.poll height-stabilization block — layout is already settled by waitForVRTReady before takeScreenshot is called. Removed the prepareForVisualRegression 1-line wrapper in favour of direct waitForVRTReady calls.

tests/playwright/vrt-dashboard.spec.ts

Replaced inline waitForFunction viewport-width boilerplate with waitForVRTReady(page, targetWidth). Removed redundant // NEW: Responsive breakpoint tests comment.

tests/playwright/vrt-timer-controls.spec.ts

Removed verbose afterEach comment — self-documenting code.

Dependency Changes Summary

• package.json changes included in PR
• pnpm-lock.yaml updated and included
• Security audit performed on new versions
• Breaking changes documented
• Testing completed with new dependencies

Dependency Matrix

Package	From	To	Type	Breaking Changes
N/A	—	—	—	—

Original prompt

---

This section details on the original issue you should resolve

<issue_title>VRT Stabilization and Standardization Action Plan</issue_title>
<issue_description>Based on cumulative audit findings across open PRs, this guide defines the standards and implementation steps required to eliminate VRT flakiness, stop threshold inflation, and keep visual baselines meaningful.

Goals

1. Make screenshots deterministic across local and CI.
2. Keep thresholds strict enough to catch real regressions.
3. Replace ad hoc waits and one-off fixes with reusable test primitives.
4. Standardize masking, setup, and teardown behavior across all Playwright VRT specs.

Non-Goals

1. Hiding layout regressions by raising maxDiffPixelRatio globally.
2. Expanding mask coverage to include stable UI just to pass CI.
3. Introducing test-only product behavior that diverges from runtime behavior.

Core Standards

1) Threshold Policy (No Inflation)

1. Default for static or mostly static captures:
   • maxDiffPixelRatio: 0.1
2. Allowed exception for complex dynamic captures (charts, dense HR overlays, combined timer+HR states):
   • maxDiffPixelRatio: 0.15 maximum
3. Color sensitivity baseline in SCREENSHOT_OPTIONS:
   • threshold: 0.2
4. Forbidden without explicit documented rationale in test comments:
   • maxDiffPixelRatio > 0.15

2) Synchronization Policy (No Arbitrary Sleeps)

1. Ban arbitrary waits:
   • Disallow waitForTimeout(500|1000|1500|...) for stabilization.
2. Required readiness checks before snapshot:
   • await page.evaluateHandle(() => document.fonts.ready)
   • await page.waitForLoadState('networkidle')
   • explicit UI assertions (toBeVisible, toHaveText, toHaveCSS)
3. For viewport-sensitive captures, require width lock:
   • await page.waitForFunction((w) => document.body.clientWidth === w, targetWidth)
4. For geometry-sensitive captures, force and verify layout:
   • await page.evaluate(() => document.body.offsetHeight)

3) Screenshot Normalization Policy

1. Always use CSS-scale screenshots:
   • scale: 'css'
2. Always disable transition noise before capture:
   • no animation, no transitions, no ripple animation artifacts.
3. Prefer deterministic clipping/viewport control over relaxed thresholds when mobile/tablet height drifts.

4) Masking Policy

1. Masks must target truly dynamic regions only.
2. Shared dashboard masking should include:
   • getDynamicContentMasks(page)
   • .variable-text-container where applicable
3. Do not mask large stable layout containers.
4. Keep mask style injection scoped and explicit.

5) State Isolation and Deterministic Mocks

1. Use resetServerState(request) in top-level beforeEach for multi-page/stateful suites.
2. Use mockLoggedInSession(context) where auth gates affect UI rendering.
3. Mock internal APIs with explicit route.fulfill payload and status.
4. Abort unstable third-party scripts (Spotify SDK) in VRT setup:
   • https://sdk.scdn.co/spotify-player.js
5. Use deterministic teardown (afterEach) for timers and long-running state.

Canonical Utility Patterns

waitForVRTReady(page) contract

Minimum responsibilities:

1. Await fonts readiness.
2. Await networkidle.
3. Force layout read/reflow.
4. Apply stabilization CSS once per page if needed.

Example shape:

export async function waitForVRTReady(page: Page): Promise<void> {
  await page.evaluateHandle(() => document.fonts.ready)
  await page.waitForLoadState('networkidle')
  await page.evaluate(() => document.body.offsetHeight)
}

Global stabilization CSS block

*, *::before, *::after {
  transition: none !important;
  animation: none !important;
}
::-webkit-scrollbar {
  display: none !important;
}
.MuiTouchRipple-root {
  display: none !important;
}

Locator-safe screenshot behavior

If screenshot target is a Locator, do not force window.scrollTo(0, 0) because MUI popovers/menus can detach.

PR Review Acceptance Rules

A VRT-related PR should be blocked if any of the following are present without strong evidence:

1. New waitForTimeout(...) used for stabilization.
2. maxDiffPixelRatio raised above policy limits.
3. Masking expanded to stable content.
4. Missing state reset/teardown in stateful tests.
5. Inconsistent selector strategy (getByRole where stable data-testid exists).

File-by-File Action Plan

tests/playwright/lib/visual.ts

• Revert global threshold defaults to policy baseline (maxDiffPixelRatio: 0.1 in shared defaults).
• Implement and consistently use waitForVRTReady(page).
• Keep scale: 'css' as non-optional default.
• Keep mask style injection ([data-vrt-mask="true"]) scoped and explicit.
• Preserve locator-safe scroll logic: only call window.scrollTo(0, 0) for full-page targets.
• Standardize dashboard mask target (workout-table-header vs workout-table-viewer) to a single canonical element.

tests/playwright/vrt-components.spec.ts

• Add `reset...

• Fixes VRT Stabilization and Standardization Action Plan #9590

---

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

[arii]

I can pinpoint exactly why this visual regression test is failing. The issue is a combination of a broken assertion logic in the visual helper and a stale snapshot baseline.

Here is the corrected tests/playwright/lib/visual.ts file using Playwright's polling mechanism to properly wait for dimensions to settle.

http://googleusercontent.com/immersive_entry_chip/0

Why This Was Failing

1. The Assertion Anti-Pattern: Your previous code used await expect(target).toHaveJSProperty('scrollHeight', await target.evaluate((node) => node.scrollHeight)).
   Because JavaScript evaluates arguments before executing the function, target.evaluate() captured the immediate, unsettled height (1000px) and then the expect instantaneously checked if the height equaled 1000px. It passed instantly, bypassing the intended wait time entirely.
2. Deterministic Polling: By using expect.poll(), we actively query the DOM over an interval. Once the height remains identical across multiple polling cycles, we know the CSS Grid/Flexbox reflows triggered by setViewportSize(MOBILE_VIEWPORT) have finished.
3. The Stale Baseline: In a previous PR (#9589), an artificial minHeight: 1038px was forced via an inline style. Now that this hack was removed to follow our new VRT Stabilization Guide, the natural height of the dashboard is 1000px.

Next Step: Because the natural height is now 1000px, your baseline image (dashboard-mobile.png) is stale. Run the following command in your terminal to accept the correct dimensions:

npx playwright test --update-snapshots --project=chromium

// ... existing code ...
if (!skipA11y) {
await checkAccessibility(target)
}

if (!isLocator) {
await page.evaluate(() => window.scrollTo(0, 0))
}

// Ensure the height is stable if it's a Locator
if (isLocator) {
let previousHeight = -1
await expect.poll(async () => {
const currentHeight = await target.evaluate((node: Element) => node.scrollHeight)
const isStable = currentHeight === previousHeight && currentHeight > 0
previousHeight = currentHeight
return isStable
}, {
message: 'Waiting for locator height to stabilize',
timeout: 3000,
intervals: [100, 250, 500] // Backoff polling to allow layout reflows
}).toBeTruthy()
}

const finalOptions = {
scale: 'css', // Prevent high-DPI (Retina) scaling mismatches in CI
// ... existing code ...

[Copilot]

I can pinpoint exactly why this visual regression test is failing. The issue is a combination of a broken assertion logic in the visual helper and a stale snapshot baseline.

Here is the corrected `t...

Added in b35a947. The expect.poll block now uses a typed locator reference (no repeated casts) and initializes previousHeight to null so the stability check explicitly requires a prior reading before declaring stable — avoiding a wasted first-cycle false-positive from the old -1 sentinel.

[github-actions]

🤖 Gemini Review Skipped

Review was skipped for the following reason: Gemini review is globally disabled

🤖 Gemini Manual Trigger Guide

[arii]

Run ./scripts/ci/run-linter.sh
🔍 Running ESLint...
Error: Replace 'spotify-select-music-button' with ⏎········'spotify-select-music-button'⏎······
Error: /home/runner/work/hrm/hrm/tests/playwright/vrt-spotify-controls.spec.ts:49:57 - Replace 'spotify-select-music-button' with ⏎········'spotify-select-music-button'⏎······ (prettier/prettier)

---

❌ ESLint failed with 1 errors and 0 warnings.

Error: Process completed with exit code 1.

[arii]

🤖 AI Technical Audit

PR Review: VRT Stabilization and Standardization (#9591)

Architectural Impact

This PR significantly hardens the Visual Regression Testing (VRT) infrastructure. By introducing waitForVRTReady and the expect.poll height stabilization logic, it shifts the strategy from "hopeful sleeping" to "deterministic readiness." The reduction of maxDiffPixelRatio from 0.4 to 0.15 is a major quality win, ensuring that layout shifts are actually caught rather than ignored by overly permissive thresholds.

Anti-AI-Slop

1. OVERLY VERBOSE COMMENTS: In lib/visual.ts, lines 35-43 (the JSDoc for waitForVRTReady) and lines 83-87 (the explanation of expect.poll) are borderline verbose but acceptable given the complexity of CI synchronization. However, the comment on line 228 is redundant as the function name is self-describing.
2. OVER-ENGINEERING: The isLocator duck-typing check ('scrollIntoViewIfNeeded' in target) is a bit fragile. While it works for Playwright's current API, it reflects a lack of strict type branching.
3. CODE RATIO / DELETIONS:
   • lib/visual.ts: The isLocator variable is declared twice (Line 71 and formerly Line 58).
   • vrt-components.spec.ts: Successfully removed the phantom screenshotOptions nesting.
   • vrt-connect-page.spec.ts: Removed waitForTimeout(100) (Excellent).
   • Redundant Code: prepareForVisualRegression in lib/visual.ts is now a 1-line wrapper for waitForVRTReady. Suggest deleting the wrapper and updating callers to use the primitive directly.
4. STALE FEATURES: The waitForFontsLoaded function in lib/index.ts appears to be superseded by waitForVRTReady but was not checked for deletion in the provided diff (only the import was removed from visual.ts).

File-by-File Analysis

lib/visual.ts

Problem: The isLocator check is redundant and the scrolling logic for non-locators is manually cast.
Implementation Sample:

// Refactor to handle branching cleaner
if ('evaluate' in target && !('scrollIntoViewIfNeeded' in target)) {
  await (target as Page).evaluate(() => window.scrollTo(0, 0));
}

vrt-dashboard.spec.ts & vrt-hr-components.spec.ts

Problem: Hardcoding viewports like 1920x1080 or MOBILE_VIEWPORT repeatedly in tests.
Implementation Sample:

// Create a helper or use a test fixture to enforce VRT viewports
// instead of calling setViewportSize in every test file.

vrt-timer-controls.spec.ts

Problem: The afterEach teardown calls stopTimer. If a test fails before the timer starts, stopTimer might hang if the UI isn't in the expected state.
Implementation Sample:

// Ensure stopTimer has a short internal timeout or check visibility first
test.afterEach(async () => {
  if (await controlPage.getByTestId('stop-button').isVisible()) {
     await stopTimer(controlPage, dashboardPage);
  }
});

Review automatically published via RepoAuditor.

[Copilot]

Run ./scripts/ci/run-linter.sh
🔍 Running ESLint...
Error: Replace 'spotify-select-music-button' with ⏎········'spotify-select-music-button'⏎······...

Fixed in 01cf3ee — the long getByTestId argument is now broken across lines to satisfy Prettier's line-length rules.

[github-actions]

🤖 Gemini Review Skipped

Review was skipped for the following reason: Gemini review is globally disabled

🤖 Gemini Manual Trigger Guide

[arii]

🤖 AI Technical Audit

Architectural Impact

This PR effectively stabilizes the Visual Regression Testing (VRT) infrastructure by shifting from flaky, time-based retries to deterministic state management and synchronization. By aligning the Next.js debug route with Express and fixing environment variable precedence in next.config.js, it ensures consistent behavior across build environments. The standardization of thresholds and the introduction of waitForVRTReady significantly reduce the risk of false positives/negatives in CI.

File-by-File Analysis

app/api/debug/reset/route.ts

• Problem: The original guard was hardcoded to development, which is incompatible with staging/CI environments where NODE_ENV is production but debug features are required for state cleanup.
• Implementation Sample:

// Improved security/flexibility
if (env.NODE_ENV === 'production' && !env.ALLOW_DEBUG_RESET) {
  return NextResponse.json({ message: 'Forbidden' }, { status: 403 });
}

next.config.js

• Problem: The existing logic effectively nullified NEXT_PUBLIC_TESTING if the TESTING build arg wasn't passed, even if the environment variable was already set in the shell. This causes window.__TEST_CONTROLS__ to fail hydration.
• Implementation Sample:

NEXT_PUBLIC_TESTING: process.env.TESTING ?? process.env.NEXT_PUBLIC_TESTING

tests/playwright/lib/visual.ts

• Problem: Global thresholds were too loose (0.4), and takeScreenshot was forcing scrolls on locators, which can cause MUI Popovers to detach or recalculate positions incorrectly.
• Implementation Sample:

const isLocator = 'scrollIntoViewIfNeeded' in target;
if (!isLocator) {
  await (target as Page).evaluate(() => window.scrollTo(0, 0));
}

ANTI-AI-SLOP DIRECTIVES

1. OVERLY VERBOSE COMMENTS: Deleted redundant comments in assertFixedDimensions and vrt-dashboard.spec.ts (e.g., "// NEW: Responsive breakpoint tests" removed).
2. OVER-ENGINEERING: None detected; the fix for NEXT_PUBLIC_TESTING is the simplest valid approach for environment precedence.
3. DUPLICATE HOOKS/TYPES: Found prepareForVisualRegression which was a redundant wrapper for waitForFontsLoaded. Successfully consolidated into waitForVRTReady.
4. CODE RATIO: The PR adds approximately 120 lines but correctly removes ~15 lines of stale or redundant code. Suggested further deletion of the catch-block console log in setup.ts to reduce noise.
5. STALE FEATURES: The retries configuration in playwright.config.ts was disabled, which is the correct action when moving to deterministic synchronization.

Review automatically published via RepoAuditor.

[arii]

File-by-File Analysis
app/api/debug/reset/route.ts
Problem: The original guard was hardcoded to development, which is incompatible with staging/CI environments where NODE_ENV is production but debug features are required for state cleanup.
Implementation Sample:
// Improved security/flexibility
if (env.NODE_ENV === 'production' && !env.ALLOW_DEBUG_RESET) {
return NextResponse.json({ message: 'Forbidden' }, { status: 403 });
}
next.config.js
Problem: The existing logic effectively nullified NEXT_PUBLIC_TESTING if the TESTING build arg wasn't passed, even if the environment variable was already set in the shell. This causes window.TEST_CONTROLS to fail hydration.
Implementation Sample:
NEXT_PUBLIC_TESTING: process.env.TESTING ?? process.env.NEXT_PUBLIC_TESTING
tests/playwright/lib/visual.ts
Problem: Global thresholds were too loose (0.4), and takeScreenshot was forcing scrolls on locators, which can cause MUI Popovers to detach or recalculate positions incorrectly.
Implementation Sample:
const isLocator = 'scrollIntoViewIfNeeded' in target;
if (!isLocator) {
await (target as Page).evaluate(() => window.scrollTo(0, 0));
}

[github-actions]

🤖 Gemini Review Skipped

Review was skipped for the following reason: Gemini review is globally disabled

🤖 Gemini Manual Trigger Guide

[Copilot]

🤖 AI Technical Audit

Architectural Impact...

All three points are already addressed in this branch: the debug/reset guard aligns with server.ts (commit 7e043e3), the next.config.js ?? fallback is in place (7e043e3), and visual.ts already has threshold: 0.2 / maxDiffPixelRatio: 0.1 with the if (!isLocator) scroll guard (from 6ead3ec and earlier commits).

File-by-File Analysis
app/api/debug/reset/route.ts
Problem: The original guard was hardcoded to development, which is incompatible with staging/CI environments where NODE_ENV is production but debug f...

Confirmed — all three fixes are in this branch. visual.ts scroll guard (if (!isLocator)) was added in an earlier commit on this branch; the thresholds are 0.2/0.1 (not 0.4). The debug/reset and next.config.js fixes landed in 7e043e3.

[arii]

✓ 26 [chromium] › tests/playwright/vrt-components.spec.ts:159:3 › Component-Specific VRT › RefreshIconButton states (6.3s)
Console info: [WebSocketProvider] Connected to server
Console log: [TimerSoundProvider] User interaction detected, initializing audio...
Console log: [AudioManager] Unlocking audio context via user interaction
Console info: [WebSocketProvider] Connected to server
Console error: Error: VRT Test Error
at u (http://127.0.0.1:3000/_next/static/chunks/3827.71f01404ef4be810.js:1:215)
at ak (http://127.0.0.1:3000/_next/static/chunks/fb83c7f9-015df68a5ab72476.js:1:53126)
at o0 (http://127.0.0.1:3000/_next/static/chunks/fb83c7f9-015df68a5ab72476.js:1:73578)
at is (http://127.0.0.1:3000/_next/static/chunks/fb83c7f9-015df68a5ab72476.js:1:84051)
at sp (http://127.0.0.1:3000/_next/static/chunks/fb83c7f9-015df68a5ab72476.js:1:128031)
at http://127.0.0.1:3000/_next/static/chunks/fb83c7f9-015df68a5ab72476.js:1:127876
at sd (http://127.0.0.1:3000/_next/static/chunks/fb83c7f9-015df68a5ab72476.js:1:127884)
at sn (http://127.0.0.1:3000/_next/static/chunks/fb83c7f9-015df68a5ab72476.js:1:123809)
at sQ (http://127.0.0.1:3000/_next/static/chunks/fb83c7f9-015df68a5ab72476.js:1:149410)
at MessagePort.M (http://127.0.0.1:3000/_next/static/chunks/4062-1ba8b9fffc874896.js:2:16717)
Console info: [useWebSocket] Manually disconnected.
Console info: [WebSocketProvider] Disconnected from server. Code: 1005, Reason:

1. [chromium] › tests/playwright/vrt-components.spec.ts:90:3 › Component-Specific VRT › SpotifyDeviceSelector menu
   Error: expect(locator).toHaveCSS(expected) failed

Locator: locator('[data-testid="spotify-device-selector-menu-paper"]').last()
Expected: "1"
Timeout: 5000ms
Error: element(s) not found

Call log:
  - Expect "toHaveCSS" with timeout 5000ms
  - waiting for locator('[data-testid="spotify-device-selector-menu-paper"]').last()
    3 × locator resolved to <div tabindex="-1" data-testid="spotify-device-selector-menu-paper" class="MuiPaper-root MuiPaper-elevation MuiPaper-rounded MuiPaper-elevation8 MuiPopover-paper MuiMenu-paper mui-18p7hyf">…</div>
      - unexpected value "0"


  145 |
  146 |     // Wait for the opacity transition to finish rendering
> 147 |     await expect(menu).toHaveCSS('opacity', '1')
      |                        ^
  148 |
  149 |     // Perform manual accessibility check on the specific menu element to ensure context validity
  150 |     await checkAccessibility(menu)
    at /home/runner/work/hrm/hrm/tests/playwright/vrt-components.spec.ts:147:24

[Copilot]

🤖 AI Technical Audit

Architectural Impact...

All three points were already in this branch — confirmed. Additionally, per the no-expanding-waits policy flagged in this audit, the waitForFunction timeout increase (3 s → 10 s) has been reverted in 1574dde.

[github-actions]

🤖 Gemini Review Skipped

Review was skipped for the following reason: Gemini review is globally disabled

🤖 Gemini Manual Trigger Guide

[github-actions]

🤖 Gemini Review Skipped

Review was skipped for the following reason: Gemini review is globally disabled

🤖 Gemini Manual Trigger Guide

[arii]

@Gemini-bot

[arii]

Reviewed commit: 5e1afecf9c36208f87f8d3b6e49eddcfa43941c4

The CI failure is due to visual regression tests, specifically for the dashboard in mobile and tablet viewports, as indicated by the binary diff for dashboard-mobile-chromium-linux.png.

The PR aims to address test flakiness by introducing waitForVRTReady and adjusting maxDiffPixelRatio. While waitForVRTReady is a good addition, the maxDiffPixelRatio for responsive dashboard tests was significantly tightened from 0.4 to 0.15. It appears that 0.15 is still too strict for the current state of stability in these complex responsive views, leading to failures.

To unblock the CI, I recommend a

Reviewed at commit: 5e1afecf9c36208f87f8d3b6e49eddcfa43941c4

---

🤖 Gemini Manual Trigger Guide

[arii]

🤖 AI Technical Audit

Technical Review - PR #9591

Architectural Impact

This PR significantly strengthens the reliability of the test infrastructure by moving from a 'retry-heavy' strategy to a 'deterministic-readiness' strategy. The introduction of waitForVRTReady and the standardization of maxDiffPixelRatio at 0.1-0.15 ensures that visual regressions are actually caught rather than ignored by loose thresholds or masked by flakiness. The fix for the Next.js env block is critical for environment parity.

Anti-AI-Slop Section

1. OVERLY VERBOSE COMMENTS: tests/playwright/vrt-timer-controls.spec.ts had a redundant comment in afterEach which was correctly removed. However, tests/playwright/vrt-dashboard.spec.ts still contains some 'Wait for visible' comments that are self-documenting.
2. OVER-ENGINEERING: The waitForVRTReady function is well-scoped. I flag the Promise.race block in vrt-dashboard.spec.ts as slightly over-engineered; explicit sequencing or a custom locator is usually cleaner than racing UI states.
3. DUPLICATE HOOKS/TYPES: prepareForVisualRegression was a duplicate of what waitForVRTReady now handles. Good deletion.
4. CODE RATIO: The PR adds significant boilerplate in vrt-dashboard.spec.ts. At least 10 lines can be saved by abstracting the Spotify/Timer visibility checks into a shared setup function.
5. STALE FEATURES: The PR successfully replaces process.env.TESTING overrides with a proper null-coalescing check in next.config.js and deletes the old retry logic.

File-by-File Analysis

app/api/debug/reset/route.ts

Problem: Security logic was tied strictly to NODE_ENV, which fails in staging/pre-prod environments intended for testing.
Implementation Sample:

// Approved change: allows explicit bypass via env var
if (env.NODE_ENV === 'production' && !env.ALLOW_DEBUG_RESET) { return 403; }

tests/playwright/lib/visual.ts

Problem: The use of window.scrollTo(0, 0) on Locator targets caused MUI Portals/Popovers to detach because they are often anchored to the viewport or specific parents.
Implementation Sample:

// Improved logic: only scroll if the target is the Page, not a specific element.
if (!isLocator) {
  await (target as Page).evaluate(() => window.scrollTo(0, 0))
}

tests/playwright/vrt-dashboard.spec.ts

Problem: The PR adds repetitive .waitFor({ state: 'visible' }) blocks for the same elements across mobile and tablet tests.
Implementation Sample:

// Suggestion: Create a helper for dashboard stability
async function waitForDashboardLoaded(page: Page) {
  await page.getByTestId('main-content-layout').waitFor();
  await page.getByTestId('timer-display-container').waitFor();
  // ... etc
}

Review automatically published via RepoAuditor.

[arii]

🤖 AI Technical Audit

Comprehensive Code Review: PR #9591

🚨 ANTI-AI-SLOP DIRECTIVES (MANDATORY) 🚨

1. OVERLY VERBOSE COMMENTS:
   • FLAGGED: In tests/playwright/timer-ui-sync.spec.ts, there are several lines of purely descriptive, AI-generated "slop" comments that narrate obvious code steps rather than explaining why (e.g., // Click while the button is still enabled, // Ensure it reverted to the stopped state).
2. OVER-ENGINEERING:
   • FLAGGED: In tests/playwright/lib/setup.ts, manually checking for ? and appending &testing=true via ternary operators is slightly over-engineered and brittle. A cleaner approach would be utilizing standard URL parsing (new URL()) or passing test states via Playwright context (like setting a secure cookie or local storage state during test setup).
3. DUPLICATE HOOKS/TYPES:
   • PASSED: The PR successfully removes the redundant prepareForVisualRegression wrapper and replaces it with the robust waitForVRTReady primitive. No new duplicate patterns were introduced.
4. CODE RATIO (>100 lines added, mandatory 10 lines to remove):
   • ACTION REQUIRED: You must delete the 9 lines of AI-generated comments in timer-ui-sync.spec.ts and the redundant inline comment in vrt-hr-components.spec.ts. See the file-by-file section for the explicit deletion targets.
5. STALE FEATURES:
   • PASSED: The old timeout-based stabilization mechanics and the prepareForVisualRegression method were successfully scrubbed from the diff.

---

🏗️ Architectural & System Impact

Overall, this PR achieves its primary directive well: migrating VRT infrastructure from an arbitrary delay-based model (retries and sleeps) to a deterministic readiness primitive (waitForVRTReady).

• Stability: Removing global retries (retries: 0) in Playwright is a fantastic architectural forcing function. It ensures that flakiness is treated as a test logic error rather than hidden by CI retries.
• Threshold Policy: Standardizing maxDiffPixelRatio around 0.1 and 0.15 aligns perfectly with the issue goals. It stops threshold inflation and delegates stabilization to layout calculation (body.offsetHeight) and networkidle.
• Security/Env Logic: Correcting the debug reset route condition safely exposes test utilities only during the right runtime phases without compromising production security.

---

📁 File-by-File Analysis

tests/playwright/timer-ui-sync.spec.ts

Problem: The file is littered with AI-generated "play-by-play" narration. We read code, not novels.

Implementation Sample:
Remove the following lines entirely:

// DELETE THESE LINES:
// Wait for either the start or stop button to appear to ensure UI is ready.
// This prevents unnecessarily polling for 2 seconds when the timer is already stopped.
// Wait until the start button is visible and enabled before continuing with the test.
// Click while the button is still enabled
// Immediately disconnect before the server's response can be processed
// Wait for the optimistic action timeout to expire
// Ensure it reverted to the stopped state

tests/playwright/lib/setup.ts

Problem: String concatenation for URL query parameters can break if hash routes (#) are present in the target route. Playwright .goto resolves against the baseURL but simple string append is risky.

Implementation Sample:

// Better alternative using native URL parser:
export async function navigateAndWait(page: Page, route: string = ''): Promise<void> {
  const baseUrl = getBaseURL()
  const url = new URL(route, baseUrl)
  url.searchParams.set('testing', 'true')
  await page.goto(url.toString())
}

tests/playwright/lib/visual.ts

Feedback: The locator scroll guard (if (!isLocator) { ...scrollTo(0, 0) }) is an excellent fix for MUI popover detachments. Using document.fonts.ready as an evaluation handle is the optimal way to handle font-loading determinism.

app/api/debug/reset/route.ts

Feedback: Logical update is sound. Using env.NODE_ENV === 'production' && !env.ALLOW_DEBUG_RESET effectively locks down production environments while providing an explicit override hatch, allowing test clusters to properly invoke the debug endpoints.

---

🔄 GitHub Checks Analysis

The automation checks report failures on Enrich PR Title and Description, Path Filter, and Check Diff.

• Correlation: These are CI workflow orchestration failures (likely missing secrets on fork/PR context or syntax errors in the GitHub Action YAML) and are not related to your TypeScript/Playwright code logic. Your code changes strictly target the test execution context and do not disrupt workflow configurations.

Review automatically published via RepoAuditor.

[github-actions]

🤖 Gemini Review Skipped

Review was skipped for the following reason: Gemini review is globally disabled

🤖 Gemini Manual Trigger Guide

[github-actions]

🤖 Gemini Review Skipped

Review was skipped for the following reason: Gemini review is globally disabled

🤖 Gemini Manual Trigger Guide

[github-actions]

🤖 Gemini Review Skipped

Review was skipped for the following reason: Gemini review is globally disabled

🤖 Gemini Manual Trigger Guide

[github-actions]

🤖 Gemini Review Skipped

Review was skipped for the following reason: Gemini review is globally disabled

🤖 Gemini Manual Trigger Guide