thread-focus-mode.spec.ts: anchor-preservation test assumes a session-wide invariant the feature doesn't guarantee - #65
Merged
Conversation
…-focus-mode spec (buzz#56) Fixes thread-focus-mode.spec.ts's "focus and split preserve reading context and interaction ownership" test, which flaked ~2/3 of isolated repeat-each runs at main HEAD (per #36's investigation). Root cause (per issue #56, product-owner call confirmed in issue comments): useThreadViewModeSwitch.changeThreadViewMode restores whatever row was top-visible at the moment of EACH toggle click — a per-toggle contract, not a session-wide one. The test captured `anchorId` once before the first toggle and reused it for both toBeInViewport() assertions, so the second assertion failed whenever intervening interactions (sidebar click, mention autocomplete open/dismiss) legitimately moved scroll position between the two toggles. Fix: fix the spec, not the product (per the dispatched call — session-wide anchor preservation is not a guarantee the feature makes). Re-capture a fresh `secondAnchorId` via topVisibleMessageId(body) immediately before the second toggle ("Expand thread"), matching what changeThreadViewMode itself does at click time. The first assertion is untouched. Out of scope (per issue): the useAnchoredScroll.ts / scrollToMessageImperative virtualized-branch side-finding — needs its own repro and issue. Files changed: - desktop/tests/e2e/thread-focus-mode.spec.ts Verification: fmt-check, desktop-tauri-fmt-check, clippy, test-unit, desktop-check, desktop-test, desktop-build, web-check, web-build all pass. Note: this sandbox has no browser/display, so the Playwright spec itself (and the ~15x --repeat-each proof the issue asks for) could not be run here. Needs CI or a reviewer with a display to confirm the flake is gone. Closes #56 Signed-off-by: Jonathan Green <dev.jonathan.green@gmail.com>
…precede anchorId/secondAnchorId were ordinal names that didn't say what each captured value was for; anchorIdBeforeFocusToggle/anchorIdBeforeSplitToggle make the intent clear without needing the comment to carry it, matching the before/after anchor naming used elsewhere in this test suite. Signed-off-by: Jonathan Green <dev.jonathan.green@gmail.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.
Fixes the flaky anchor-preservation assertion in
thread-focus-mode.spec.tsby re-capturing the scroll anchor before the second toggle instead of assuming it stays valid across the whole test session, and renames the anchor variables to reflect which toggle each one precedes.Part of #56
This PR was produced by the sandcastle
agent:implementrunner and is awaiting human review.🤖 Generated with Claude Code