Add a diagnostic test for revealing a window moved into a four-column workspace - #204
Add a diagnostic test for revealing a window moved into a four-column workspace#204lamdor wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 36 minutes Limit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
74fcb0f to
c14d2eb
Compare
…workspace A window moved to an adjacent workspace can land selected in a column outside the destination viewport: parked offscreen, given no frame write, yet holding keyboard focus. These tests record the viewport state observed in that reproduction and the reveal behavior around it. The fixture is the destination geometry from the reproduction: four 1150pt columns with a 20pt gap at x = 0, 1170, 2340, 3510, seen through a 2560pt viewport, with the moved window landing in the last column. In that state the rebase step of ensureSelectionVisible leaves viewOffsetPixels at -3530 with viewStart at -20, so column 3 starts roughly 970pt beyond the right viewport edge. Five tests assert the reveal contract under disabled motion: the rebase is view-neutral, the moved column is parked, a parked destination column is revealed, a scroll is scheduled, and scroll lock does not exempt a fully parked target. Three further tests separate the two motion configurations, because they behave differently in a way that matters for the surrounding code. With motion disabled, animateToOffset takes its static fallback and both current and target land on the snap. With motion enabled — the value MotionPolicy.snapshot() returns, so the one the runtime move path uses — it installs a spring: the target moves while the current offset stays put for the display-link driver to interpolate. The plan-build step in NiriLayoutHandler tests abs(viewOffsetPixels.current() - offsetBefore) > 1 to decide whether to emit a .startNiriScroll directive, so it observes the former but not the latter. These are diagnostic rather than a regression guard for a landed fix. The runtime defect is timing-dependent and does not reproduce under trace capture.
c14d2eb to
db1ecef
Compare
|
Context after the investigation that produced this PR concluded. The bug these tests were written to diagnose is fixed by #209: the AX focus-confirmation path was skipping the reveal on a token-only re-confirmation test, so the destination viewport was preserved and the moved window stayed outside it. What these tests are worth now. They are no longer diagnostic — they pin real contracts on the reveal path that the investigation established:
None of these assert the fixed bug's behaviour, so they do not overlap with the tests in #209. They do document the geometry from the reproduction, which is why the fixture uses those exact column widths and viewport size. CI green on Note #207 depends on this PR — it modifies the test file added here, so it cannot be rebased onto Marking ready for review. |
|
Merge order: this PR before #207. #207 modifies the test file this PR adds ( The other PRs from this investigation are independent and can merge in any order: #209 (the actual fix), #202, #203, #206. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| var fixture = rebasedFixture() | ||
| let offsetBefore = fixture.state.viewOffsetPixels.current() | ||
|
|
||
| _ = fixture.reveal(movedColumn, trigger: .automatic, motion: .enabled) | ||
|
|
||
| let currentDelta = abs(fixture.state.viewOffsetPixels.current() - offsetBefore) | ||
| let targetDelta = abs(fixture.state.viewOffsetPixels.target() - offsetBefore) | ||
|
|
||
| #expect(currentDelta <= 1) | ||
| #expect(targetDelta > 1) | ||
| #expect(fixture.state.viewOffsetPixels.isAnimating) | ||
| } | ||
|
|
There was a problem hiding this comment.
This test records that an enabled-motion reveal changes the target while leaving the current offset unchanged, but it bypasses layout-plan construction and never checks whether .startNiriScroll reaches the display-link driver. The suite therefore passes even when the subsequent production boundary drops the scheduled scroll, limiting its ability to localize the reported defect.
Prompt To Fix With AI
This is a comment left during a code review.
Path: Tests/NehirTests/MovedWindowRevealAfterWorkspaceTransferTests.swift
Line: 134-146
Comment:
**Diagnostic stops before directive emission**
This test records that an enabled-motion reveal changes the target while leaving the current offset unchanged, but it bypasses layout-plan construction and never checks whether `.startNiriScroll` reaches the display-link driver. The suite therefore passes even when the subsequent production boundary drops the scheduled scroll, limiting its ability to localize the reported defect.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
There was a problem hiding this comment.
Accurate, and worth stating plainly: these tests do not reach the directive-emission boundary, and they did not localize the defect.
The actual cause turned out to be earlier than either boundary. The AX focus-confirmation path skipped the reveal entirely on a token-only re-confirmation test, so scrollToReveal never ran and there was no scheduled scroll for the plan-build gate to drop. That is fixed in #209 and confirmed in a real reproduction. The gate blindness this test pair documents is real and measured, but downstream of the cause — see #207, which is open with the same caveat.
So the scope limit is genuine. What I would push back on slightly is the framing that it should have been extended: driving layout-plan construction to assert .startNiriScroll reaches the display-link driver needs a controller-level fixture with a live WMController, a monitor set and a refresh pipeline. That is a much heavier test, and given the failure was two boundaries upstream it would not have found it either.
What these tests are worth now is narrower than when they were written, and I have said so on the PR: they pin the view-neutral rebase, parked-column classification, scroll-lock behaviour on a parked target, and the two motion configurations differing. None of that overlaps #209's tests.
If the maintainers would rather not carry tests whose diagnostic purpose has passed, this PR is fine to close — #207 then needs rebasing to carry the test-file changes itself.
There was a problem hiding this comment.
That's a fair correction on both counts. The framing implied the tests should have gone further, which overstates the obligation — reaching .startNiriScroll requires a controller-level fixture that's a qualitatively different investment, and the failure being two call-sites upstream means it wouldn't have localized the defect anyway.
The remaining value you describe is real: the rebase invariant, parked-column classification, scroll-lock carve-out, and the motion-path signal asymmetry are all pinned independently of #209, and none of them duplicate what that fix's tests cover. The asymmetry test in particular is a useful artifact even now — it documents why currentDelta <= 1 is an unreliable gate signal for an animated scroll, which is non-obvious.
The "close vs. rebase onto #207" question is a maintainer call. If the goal is to keep those behavior pins, landing them here and having #207 reference them is clean. If the PR's framing as a diagnostic for a now-resolved defect makes it awkward to carry long-term, folding the file into #207 is equally reasonable. Either way the comment's scope limitation is accurate, and I wouldn't restate it as a blocker.
I'll withdraw the concern as a review issue — it's a documented design decision, not a defect.
There was a problem hiding this comment.
That context changes the picture considerably — thanks for writing it up.
The comment stands as accurate about the scope boundary, but your pushback on the framing is fair. The failure was upstream of both boundaries the tests exercise, so extending to controller-level fixture and directive assertion would have passed without finding it regardless. That reframes the limit as structural to what unit tests can cover here, not a missed extension.
The residual value you name — rebasing neutrality, parked-column classification, scroll-lock exemption, and the two motion paths — is concrete and non-overlapping with #209's tests. That is worth pinning even after the diagnostic purpose has passed, unless there is a project preference to retire tests once their motivating defect is resolved.
Whether to carry this forward as #207 takes the file, or close it, is a maintainer call — but the tests are not wrong, and the scope limit is documented. No changes needed from me here.
1 participant
Why
A window moved to another workspace can end up selected in a column outside the destination viewport: parked offscreen, given no frame write, yet holding keyboard focus. The user sees a different window than the one they moved, while keystrokes go to the moved window.
The runtime defect is timing-dependent — it does not reproduce while trace capture is active, and reproduces most of the time when it is off. That makes runtime instrumentation unreliable for locating it, so this records the observed state as a deterministic test instead.
What this records
The fixture is the destination geometry from a real reproduction: four 1150pt columns with a 20pt gap at
x = 0, 1170, 2340, 3510, seen through a 2560pt viewport, with the moved window landing in the last column.In that state the rebase step of
ensureSelectionVisibleleavesviewOffsetPixelsat-3530withviewStartat-20— verified by reproducing the arithmetic from the captured values. Column 3 therefore starts roughly 970pt beyond the right viewport edge, andcolumnVisibilityclassifies it.parked(.maximum).The tests assert what should then happen:
rebasingActiveColumnPreservesViewStart— the rebase is view-neutral (this is by design, and is what makes the follow-up reveal load-bearing)movedColumnIsParkedBeforeReveal— the moved column is fully outside the viewportrevealsMovedColumnParkedOutsideDestinationViewport— a parked destination column is revealedschedulesScrollTowardMovedColumn— a scroll is actually queued (the reproduction showedcurrentViewStart == targetViewStart == -20, i.e. nothing queued)revealsMovedColumnEvenWhenViewportIsScrollLocked— scroll lock does not exempt a fully parked target, per its own documented contractStatus
Diagnostic, not a regression guard for a landed fix. No fix accompanies it. The point is to determine deterministically whether
scrollToRevealmishandles this state, or whether the reveal result is computed correctly and then lost before the layout pass consumes it.Source analysis of
scrollToRevealagainst these exact numbers suggests every snap candidate for column 3 yields a target offset far from-3530(leftEdge→-20,rightEdge→-1390,center→-705, against a 0.5pt tolerance), so the reveal should schedule a scroll. If these tests pass, that points at the commit/ordering path rather than the reveal logic.Note this cannot be run locally without Xcode (Swift Testing ships with it); opening as a draft so CI runs the suite.
Based on
mainrather than on #202/#203 so the result is not affected by those changes.Greptile Summary
This PR adds a deterministic Swift Testing fixture for the viewport state observed after moving a window into a four-column workspace and records the file in provenance metadata.
Confidence Score: 4/5
The PR appears safe to merge, with the non-blocking limitation that its diagnostic stops before the production layout directive path it is intended to help distinguish.
The fixture correctly exercises viewport rebasing, visibility, and reveal state transitions, but it can pass without demonstrating that the scheduled animated scroll survives layout-plan construction and reaches the display-link driver.
Files Needing Attention: Tests/NehirTests/MovedWindowRevealAfterWorkspaceTransferTests.swift
Important Files Changed
Sequence Diagram
sequenceDiagram participant Transfer as Workspace transfer participant Reveal as ensureSelectionVisible / scrollToReveal participant State as ViewportState participant Plan as Layout plan participant Driver as Display-link driver Transfer->>Reveal: Move selection into destination column Reveal->>State: Install spring target State-->>Reveal: current unchanged, target changed Reveal-->>Plan: Return scheduled reveal state Plan->>Driver: Emit startNiriScroll directive Note over Reveal,State: New tests stop here Driver->>State: Interpolate current toward targetPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "Add diagnostic tests for revealing a win..." | Re-trigger Greptile