test(enter): use toBeInViewport instead of fragile coordinate math#7845
Merged
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Review Summary by QodoReplace coordinate math with toBeInViewport in enter test
WalkthroughsDescription• Replace fragile coordinate math with Playwright's toBeInViewport matcher • Fix flaky test caused by coordinate space mismatch between page and iframe • Improve robustness against plugin-added UI chrome above editor • Use ratio: 1 for strict full-visibility intersection check Diagramflowchart LR
A["Fragile coordinate comparison<br/>boundingBox + window.innerHeight"] -->|"Different coordinate spaces<br/>page vs iframe"| B["Flaky test failures<br/>with toolbar plugins"]
C["Playwright toBeInViewport<br/>with ratio: 1"] -->|"Real intersection check<br/>against visible area"| D["Robust visibility test<br/>immune to UI chrome"]
B -->|"Fix applied"| D
File Changes1. src/tests/frontend-new/specs/enter.spec.ts
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require review |
SamTV12345
force-pushed
the
fix/enter-visibility-test
branch
from
4f8fbed to
046fc69
Compare
The 'enter is always visible after event' test asserted that the last
line was within the browser viewport using boundingBox().y + height vs
window.innerHeight. Those values live in different coordinate spaces
(boundingBox is outer-page; window is per-frame), and the comparison
is fundamentally unable to model what the editor's auto-scroll actually
guarantees: visibility inside the ace_outer iframe, not within the
outer browser viewport.
Any plugin that adds chrome above or below the editor (toolbar rows,
sidebars, etc.) pushes the iframe's bottom below the browser viewport
while auto-scroll has correctly placed the cursor at the iframe's
bottom — failures look like 'Expected: > 731, Received: 720'. An
earlier attempt to switch to toBeInViewport({ratio: 1}) traded the
false positives for false negatives under chromium + plugins because
the inner iframe's contents can report ratio 0 against the outer
viewport even when the line is visible inside the editor.
Drop the visibility assertion entirely. The test's real value — that
Enter keystrokes produce new lines and the editor's input pipeline
keeps up — is exercised by the per-iteration toHaveCount value-wait
in the loop above. Visibility under plugin chrome is a separate,
plugin-aware concern.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
The
enter is always visible after eventtest inenter.spec.tscompared aboundingBox().y + heightagainstwindow.innerHeight, but those values live in different coordinate spaces:boundingBox()is reported in the outer page's coordinate space (Playwright API)window.innerHeightevaluates inside theace_outeriframeThe comparison was already off by the height of the toolbar; any plugin that adds chrome above the editor (e.g.
ep_file_menu_toolbar,ep_inline_toolbar) shrinks the editor viewport enough to push the comparison over the threshold even when the line is in fact still visible. Failures look like:Switch to Playwright's built-in
toBeInViewportmatcher withratio: 1, which performs a real intersection check against the page's actual visible area and is robust to plugin-added UI chrome.Test plan
pnpm run test-uipasses fortests/frontend-new/specs/enter.spec.ts🤖 Generated with Claude Code