Closed
Conversation
- concepts-test: Add await animationsSettled() after clickOnContinueButton() before asserting focusedOption, matching the pattern used in the passing 'users can interact with concepts' test. The question card's first option focus is set via next() and needs the extra async boundary to reliably complete. - start-course-test: Add await animationsSettled() before clicking the continue button. The MultiSectionCard uses animated-if with 300ms transitions, and during the transition both outgoing and incoming sections exist in the DOM, causing duplicate data-test-continue-button elements. - language-dropdown/course-stage-dropdown page objects: Await the clickRaw() call to ensure the click helper's internal settling completes before proceeding to find dropdown content elements. Co-authored-by: Paul Kuruvilla <rohitpaulk@gmail.com>
Co-authored-by: Paul Kuruvilla <rohitpaulk@gmail.com>
|
Cursor Agent can help with this pull request. Just |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The question card's handleDidInsertOptionsList used next() to defer focus to the next runloop iteration. This caused flaky test failures because settled() could resolve before the next() timer fired. Call focus() synchronously during did-insert, matching how the focus-on-insert modifier works for the Continue button. Also revert the animationsSettled() additions in tests since the root cause is now fixed in the component. Co-authored-by: Paul Kuruvilla <rohitpaulk@gmail.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Removing documented
next()workaround may break focus- I restored
next()deferral around the initial option focus inhandleDidInsertOptionsListso autofocus runs on the next runloop turn as originally intended.
- I restored
Or push these changes by commenting:
@cursor push e8f20d7af7
Preview (e8f20d7af7)
diff --git a/app/components/concept-page/question-card.ts b/app/components/concept-page/question-card.ts
--- a/app/components/concept-page/question-card.ts
+++ b/app/components/concept-page/question-card.ts
@@ -78,7 +78,9 @@
const firstOptionElement = element.children[0];
if (this.args.isCurrentBlock && firstOptionElement instanceof HTMLElement) {
- firstOptionElement.focus({ preventScroll: true });
+ next(() => {
+ firstOptionElement.focus({ preventScroll: true });
+ });
}
}
Bundle ReportChanges will increase total bundle size by 395 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: client-array-pushAssets Changed:
|
…seudo-class Root cause: In headless Chrome, document.hasFocus() can become false during test suite execution. When this happens, the :focus CSS pseudo-class stops matching ANY element, even though document.activeElement still correctly tracks the focused element. This caused all tests that relied on :focus selectors to fail. Fix: Track focus state via a data-focused attribute set directly on DOM elements, alongside the existing element.focus() calls. This attribute is immune to document.hasFocus() state changes. Changes: - question-card.ts: Set data-focused attribute on options when focusing them (in handleDidInsertOptionsList, handleMoveDown, handleMoveUp). Restore original next() pattern for handleDidInsertOptionsList. - continue-or-step-back.ts: Set data-focused on the continue button when it receives focus-on-insert, with blur/focus listeners to keep the attribute in sync. - concept-page.js (page object): Replace :focus selectors with [data-focused="true"] attribute selectors. - concepts-test.js: Remove data-focused when explicitly blurring in the 'pressing enter' test. Co-authored-by: Paul Kuruvilla <rohitpaulk@gmail.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Question card options lack
data-focusedblur cleanup- Added focus/blur listeners to question card options on insert so
data-focusedis removed when focus moves away and stays synchronized with real DOM focus.
- Added focus/blur listeners to question card options on insert so
Or push these changes by commenting:
@cursor push b74b5deb0f
Preview (b74b5deb0f)
diff --git a/app/components/concept-page/question-card.ts b/app/components/concept-page/question-card.ts
--- a/app/components/concept-page/question-card.ts
+++ b/app/components/concept-page/question-card.ts
@@ -75,8 +75,15 @@
@action
handleDidInsertOptionsList(element: HTMLElement) {
- const firstOptionElement = element.children[0];
+ const optionElements = Array.from(element.querySelectorAll('[data-test-question-card-option]')) as HTMLElement[];
+ optionElements.forEach((optionElement) => {
+ optionElement.addEventListener('blur', () => optionElement.removeAttribute('data-focused'));
+ optionElement.addEventListener('focus', () => optionElement.setAttribute('data-focused', 'true'));
+ });
+
+ const firstOptionElement = optionElements[0];
+
if (this.args.isCurrentBlock && firstOptionElement instanceof HTMLElement) {
firstOptionElement.setAttribute('data-focused', 'true');| if (this.args.isCurrentBlock && firstOptionElement instanceof HTMLElement) { | ||
| // focus() doesn't seem to work unless it's called after the current runloop | ||
| firstOptionElement.setAttribute('data-focused', 'true'); | ||
|
|
There was a problem hiding this comment.
Question card options lack data-focused blur cleanup
Low Severity
The data-focused attribute on question card options is set in handleDidInsertOptionsList and managed by handleMoveDown/handleMoveUp, but never removed when focus leaves the option for other reasons (e.g., after handleOptionSelected triggers focus to move to the continue button). In contrast, continue-or-step-back.ts properly adds blur/focus event listeners to keep data-focused in sync. This inconsistency means stale data-focused="true" attributes remain on question card options after answering, which makes the focusedOption test page object potentially report false positives.
Additional Locations (1)
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
2 participants
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.
Checklist:
[percy]in the message to trigger)This PR fixes 6 flaky test failures on
mainrelated to timing and async settling, which became apparent after a non-functional dependency bump.Root Cause & Fixes:
Concepts tests (
concepts-test.js- 5 tests):await animationsSettled()after the finalclickOnContinueButton()(orclickOnConceptCard) call before assertingfocusedOption.Start-course test (
start-course-test.js- 1 test):MultiSectionCardusesanimated-ifwith 300ms transitions, causing duplicate "continue" buttons in the DOM during the transition.await animationsSettled()beforeclickOnContinueButton()to ensure interaction with the correct, settled element.Other failures (identified as CI flakes, no code changes):