fix(TimePicker): stabilize flaky time-picker e2e tests#2516
Conversation
The custom-time-range and relative-time e2e specs were flaky (and the reopen test hard-failed) due to a stale-state bug in the TimePicker and non-deterministic Playwright helpers. App fix: - isRelative was only seeded from defaultRelativeTimeMode at mount, so the relative/absolute toggle silently diverged from URL state when intervals changed. Sync it when the prop becomes true (force on only -- the prop is a lossy signal that is also false for relative mode at the default live-tail interval). Test fixes: - open(): end in a stably-open state with a settle+retry loop instead of returning a popover that is mid-close. - apply(): wait for the popover to close and the network to settle so a following open() starts from a settled state. - toggleRelativeTimeSwitch / selectTimeInterval / isRelativeTimeEnabled: retry or fail fast instead of hanging to the 60s test timeout. - custom-time-range: reset the persisted hdx-time-picker-mode atom in beforeEach so a prior test's mode cannot leak across the shared worker. - relative-time: open the popover at the start of each loop iteration rather than relying on trailing state.
🦋 Changeset detectedLatest commit: f81af35 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
github-actions
Bot
added
the
review/tier-2
🔵 Tier 2 — Low RiskSmall, isolated change with no API route or data model modifications. Why this tier:
Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns. Stats
|
Greptile SummaryThis PR fixes three root causes of flaky
Confidence Score: 5/5Safe to merge — the app change is a minimal, well-bounded useEffect touching only internal toggle state, and the test changes are additive retry/settle logic with no changes to production paths. The React fix is narrow (one useEffect, one state variable, no new props or external dependencies) and the documented asymmetric sync rule prevents the known false-negative case. Test helper changes are purely additive and verified across 16 consecutive local runs including parallel workers. No production logic outside the TimePicker component is touched. No files require special attention; the one minor inconsistency (unguarded networkidle in open()) is a defensive hardening opportunity rather than a current breakage. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Spec as Test Spec
participant TPC as TimePickerComponent
participant Page as Playwright Page
participant App as TimePicker (React)
Note over Spec,App: open() — settle+retry loop
Spec->>TPC: open()
TPC->>Page: waitForLoadState('networkidle')
loop up to 5 attempts
TTC->>TPC: isPopoverStablyOpen()?
alt stably open
TPC-->>Spec: return (popover ready)
else not stable
TPC->>App: waitFor(hidden, 2s)
TPC->>Page: keyboard.press('Escape')
TPC->>App: pickerInput.click()
TPC->>TPC: isPopoverStablyOpen()?
end
end
Note over Spec,App: apply() — wait for settled close
Spec->>TPC: apply()
TPC->>App: pickerApplyButton.click()
App->>App: handleApply → close()
TPC->>App: waitFor(hidden, 5s)
TPC->>Page: waitForLoadState('networkidle')
TPC-->>Spec: return
Note over Spec,App: TimePicker.tsx — isRelative sync fix
App->>App: defaultRelativeTimeMode prop changes (→ true)
App->>App: useEffect fires → setIsRelative(true)
Note right of App: Never forced OFF (prop is lossy)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Spec as Test Spec
participant TPC as TimePickerComponent
participant Page as Playwright Page
participant App as TimePicker (React)
Note over Spec,App: open() — settle+retry loop
Spec->>TPC: open()
TPC->>Page: waitForLoadState('networkidle')
loop up to 5 attempts
TTC->>TPC: isPopoverStablyOpen()?
alt stably open
TPC-->>Spec: return (popover ready)
else not stable
TPC->>App: waitFor(hidden, 2s)
TPC->>Page: keyboard.press('Escape')
TPC->>App: pickerInput.click()
TPC->>TPC: isPopoverStablyOpen()?
end
end
Note over Spec,App: apply() — wait for settled close
Spec->>TPC: apply()
TPC->>App: pickerApplyButton.click()
App->>App: handleApply → close()
TPC->>App: waitFor(hidden, 5s)
TPC->>Page: waitForLoadState('networkidle')
TPC-->>Spec: return
Note over Spec,App: TimePicker.tsx — isRelative sync fix
App->>App: defaultRelativeTimeMode prop changes (→ true)
App->>App: useEffect fires → setIsRelative(true)
Note right of App: Never forced OFF (prop is lossy)
Reviews (4): Last reviewed commit: "Merge branch 'main' into warren/fix-flak..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private async isPopoverStablyOpen(): Promise<boolean> { | ||
| if (!(await this.pickerPopover.isVisible())) return false; | ||
| // Dwell long enough to outlast the post-apply / post-select re-render | ||
| // cascade (URL update + results reload) that can close a just-opened | ||
| // popover. Sample twice so a mid-cascade flicker is caught. | ||
| await this.page.waitForTimeout(250); | ||
| if (!(await this.pickerPopover.isVisible())) return false; | ||
| await this.page.waitForTimeout(250); | ||
| return await this.pickerPopover.isVisible(); | ||
| } |
There was a problem hiding this comment.
isPopoverStablyOpen
waitForTimeout is explicitly discouraged in Playwright because it adds a hard delay regardless of actual app state — here 500 ms of unconditional waiting per call, multiplied by every open() invocation on an already-open popover. The goal (detecting that the popover stays visible through a re-render cascade) could instead be expressed as a short expect(locator).toBeVisible() assertion with a tight timeout polled at Playwright's own rate, which would resolve immediately once stability is confirmed rather than always burning the full 500 ms. That said, the two-sample approach is documented and the behaviour is correct; this is a test-speed and style concern rather than a correctness issue.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| let lastError: unknown; | ||
| for (let attempt = 0; attempt < 4; attempt++) { | ||
| try { | ||
| await intervalButton.waitFor({ state: 'visible', timeout: 5000 }); | ||
| await intervalButton.click({ timeout: 5000 }); | ||
| return; | ||
| } catch (error) { | ||
| lastError = error; | ||
| // The popover may have closed (e.g. a swallowed render); make sure it | ||
| // is open again before the next attempt. | ||
| await this.open(); | ||
| } | ||
| } | ||
| throw lastError; |
There was a problem hiding this comment.
open() call on the final catch iteration
On the fourth (last) loop pass, when the click still fails, the catch block calls this.open() and then the loop exits — no further attempt will use the freshly opened popover. The call is harmless but leaves the popover in an unexpected open state right before throw lastError surfaces the error to the caller, which could make post-failure diagnostics (e.g. screenshots) show a spurious open picker. Consider guarding with if (attempt < 3) or hoisting the open() to the top of each iteration (similar to the pattern used in relative-time.spec.ts) so every retry starts from a known state without the trailing wasted open.
E2E Test Results✅ All tests passed • 226 passed • 3 skipped • 1530s
Tests ran across 4 shards in parallel. |
traces-workflow types a query into the search input (rendering the autocomplete 'Searching for:' portal) and then opens the time picker. That portal overlays the time-picker input and intercepted the toggle click, hard-failing the test on all retries. open() now presses Escape from its closed baseline before clicking the input, dismissing any blocking overlay. Escape is only pressed when the popover is already closed, so it cannot race a just-opened popover shut.
Deep Review✅ No critical issues found. No P0/P1 ship-blockers: the production change is a narrowly-scoped, well-commented one-way state sync, and the rest is Playwright test-helper hardening. The items below are recommendations and nits. 🟡 P2 -- recommended
🔵 P3 nitpicks (5)
Reviewers (9): correctness, testing, maintainability, project-standards, kieran-typescript, julik-frontend-races, adversarial, agent-native, learnings-researcher. Testing gaps:
|
2 participants
Why
The
custom-time-rangeandrelative-timesearch e2e specs are flaky on CI —custom-time-rangeeven hard-fails on all retries on some runs (e.g. this Shard 3 run). The flakiness is unrelated to the PR that surfaced it; it's a latent bug in theTimePickerplus non-deterministic Playwright helpers.Root causes
isRelativewas seeded fromdefaultRelativeTimeModeonly at mount, so the relative/absolute toggle silently diverged from URL state when the live interval changed — the picker rendered in a mode that no longer matched the URL.open()helper. Clicking the input toggles the popover. Right after a programmatic close (apply/select), a click could be swallowed by a re-render (popover stays shut) or land while the popover was still detaching (flicker open→closed), so the date inputs / interval buttons / relative switch were "not found".localStorageviaatomWithStorage('hdx-time-picker-mode'), so a prior test that switched to "Around a time" leaked into the reopen test (only one date input rendered →endDateInputnever resolved). This is what madecustom-time-rangehard-fail.isChecked()/ clicking a re-rendering button).What changed
App —
TimePicker.tsxisRelativetodefaultRelativeTimeModewhen the prop becomes true. Force on only: the prop is a lossy signal (alsofalsefor relative mode at the default live-tail interval), so syncing back tofalsewould wrongly drop the user out of relative mode when they pick the default interval.Tests —
TimePickerComponent.tsopen()now ends in a stably-open state via a settle+retry loop (handles already-open, closed, mid-close, and swallowed-click cases).apply()waits for the popover to close + network to settle so a followingopen()starts settled.toggleRelativeTimeSwitch/selectTimeInterval/isRelativeTimeEnabledretry or fail fast instead of hanging.Tests — specs
custom-time-range: reset the persistedhdx-time-picker-modeatom inbeforeEachfor isolation.relative-time: open the popover at the start of each loop iteration instead of relying on trailing state.Verification (local, full-stack e2e)
custom-time-range: 4 consecutive clean runsrelative-time: 4 consecutive clean runs--workers=2(CI parity): 4 consecutive clean runs (16/16)search/e2e directory at--workers=2: 65/65 passing (covers every spec using the sharedTimePickerComponent)tsc --noEmitclean; eslint cleanA changeset is included (
@hyperdx/apppatch).