Reveal a window re-focused after moving to another workspace - #209
Reveal a window re-focused after moving to another workspace#209lamdor wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 52 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 (7)
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 |
A window moved to another workspace could stay parked off the destination viewport while still holding keyboard focus, so the user saw a different window than the one they moved and keystrokes went somewhere invisible. The AX focus-confirmation path preserves the active viewport when it sees a re-confirmation of the already-confirmed focus token, because a quick-terminal hide can make macOS re-focus the existing managed window and scrolling then would yank the viewport back to a column the user deliberately scrolled away from. That test compared only tokens. Moving a window that already holds focus produces the same signature: the destination emits a focusedWindowChanged notification for a token that is already the confirmed focus, so the reveal was skipped. But the moved window's column is wherever the transfer appended it, usually outside the destination viewport, so preserving that viewport left it parked offscreen. Record the workspace the confirmed focus was established in and require it to match before treating a re-confirmation as in place. A same-window re-focus within one workspace still preserves the viewport; the same window re-focused after changing workspace now reveals. The workspace has to travel with the focus token through the reconcile cycle, so focusedWorkspaceId is carried on FocusSessionSnapshot, set by StateReducer.managedFocusConfirmed from the workspaceId it previously discarded, cleared alongside focusedToken, and round-tripped through focusSessionSnapshot and applyReconciledFocusSession. Recording it only on the live session state is not enough: confirmations arrive via confirmManagedFocus, and applyReconciledFocusSession would overwrite the field from a snapshot that did not carry it. The skip trace now also carries confirmedFocusWorkspaceUnchanged and the previous confirmed-focus workspace, so this decision is visible without new instrumentation.
11f8969 to
39b5022
Compare
Covers the contract the reveal fix depends on: confirming focus records the workspace it was confirmed in, re-confirming the same window in another workspace moves that record, re-confirming in the same workspace leaves it alone, and the value survives further focus-session writes. That last case is the one an earlier attempt at the fix got wrong. Confirmations arrive through confirmManagedFocus, which routes through the reducer, and applyReconciledFocusSession replaces the focus session wholesale from a FocusSessionSnapshot — so a field recorded only on the live session state is reset on the next reconcile and the guard never engages.
a92b4fe to
0271f37
Compare
|
Confirmed fixed by the reporter in their real reproduction. Tested with a build containing only this change — no other in-flight fix — so the confirmation attributes to this guard change alone. Note the fix required a correction after a first inert attempt: recording the confirmed-focus workspace on the live session state only was reset on every reconcile, because confirmations arrive through Tests added now that the behaviour is confirmed, in a new per-behaviour file ( Supersedes #203 and #207 as the fix for this symptom; both are open with comments explaining that their paths were never observed to execute. The investigation is written up in the discovery document in #206, including the eight candidate mechanisms eliminated before this one. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /// Workspace the confirmed focus was established in. Distinguishes a same-window | ||
| /// re-focus *in place* from the same window being re-focused after moving to a | ||
| /// different workspace, which must still reveal. | ||
| var focusedWorkspaceId: WorkspaceDescriptor.ID? |
There was a problem hiding this comment.
When the test target builds, RestorePlannerTests calls the synthesized FocusSessionSnapshot initializer without the new required focusedWorkspaceId argument, causing the test target to fail compilation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: Sources/Nehir/Core/Reconcile/ReconcileSnapshot.swift
Line: 151
Comment:
**Snapshot initializer contract broken**
When the test target builds, `RestorePlannerTests` calls the synthesized `FocusSessionSnapshot` initializer without the new required `focusedWorkspaceId` argument, causing the test target to fail compilation.
---
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.
Not valid — the field is optional with an implicit default, so the synthesized memberwise initializer does not require it.
focusedWorkspaceId is declared var focusedWorkspaceId: WorkspaceDescriptor.ID?. Swift gives an Optional stored property a default of nil in the memberwise initializer, so RestorePlannerTests.swift:80 compiles unchanged and gets focusedWorkspaceId: nil.
Verified in CI rather than by reading: the run on this branch's head compiled RestorePlannerTests.swift ([410/427] Compiling NehirTests RestorePlannerTests.swift), started and passed Suite RestorePlannerTests, and completed 145 suites with both jobs green.
nil is also the correct value for that fixture: the test constructs a snapshot for monitor-removal restore planning where no focus workspace has been established, and the guard consuming this field treats nil as "no recorded workspace" and preserves the previous behaviour.
The root cause is identified and the fix is confirmed by the reporter, so the document moves out of discovery per the branch lifecycle. Retitled Completed, and the banner now records that the fix is open as Nehir PR apphane-dev#209 — ready for review and green in CI — but not yet merged to main, so a later reader does not assume it has landed. Provenance wording now names main at f097f35 as the pre-fix state and warns that the fix itself changes the focus-confirmation lines the document cites.
1 participant
Symptom
A window moved to another workspace could stay parked off the destination viewport while still holding keyboard focus — the user saw a different window than the one they moved, and keystrokes went somewhere invisible.
Root cause
The AX focus-confirmation path preserves the active viewport when it sees a re-confirmation of the already-confirmed focus token. That guard exists for a good reason, stated in its own comment: a quick-terminal hide can make macOS re-focus the existing managed window, and scrolling then would yank the viewport back to a column the user deliberately scrolled away from.
But the test compared only tokens:
Moving a window that already holds focus produces exactly that signature — the destination workspace emits a
focusedWindowChangednotification for a token which is already the confirmed focus. SopreserveActiveViewportReasonbecame.alreadyConfirmedFocusedWindowChangedand the reveal was skipped. The moved window's column, however, is wherever the transfer appended it — usually outside the destination viewport — so preserving that viewport left the window parked offscreen.Captured from a runtime trace of the failing move:
With the destination's columns at x = 0, 1170, 2340, 3510 and a 2560pt viewport resting at −20, column 3 starts roughly 970pt beyond the right edge.
currentViewStart == targetViewStartconfirms no scroll was ever scheduled — the reveal did not run at all.Change
Record the workspace the confirmed focus was established in (
focusedWorkspaceIdon the focus session, set whereverfocusedTokenis set and cleared with it), and require it to match before treating a re-confirmation as "in place":The invariant: a same-window re-focus within one workspace preserves the viewport; the same window re-focused after changing workspace reveals. The quick-terminal case the guard was built for is unaffected — that re-focus happens within one workspace. An absent recorded workspace preserves the previous behavior, so nothing changes before the first confirmation.
The existing skip trace now also carries
confirmedFocusWorkspaceUnchangedand the previous confirmed-focus workspace name, so this decision is visible in future traces without new instrumentation.Status
Candidate fix, not confirmed. The mechanism is identified from a runtime trace of the failing move — unlike earlier attempts, the trace names the skip and its reason directly. But the runtime behaviour has not yet been confirmed fixed in a real reproduction. Draft until it has been.
Tests are deferred per
docs/TESTING.mduntil the behaviour is confirmed.Supersedes
This replaces #203 and #207 as the candidate fix for this symptom:
diff.focusedFrame. Closes a real but separate gap.Six further candidate mechanisms were investigated and eliminated; they are recorded in the discovery document in #206, which should be updated once this is confirmed.
Greptile Summary
The PR records the workspace associated with confirmed managed focus so that re-focusing a moved window reveals it without disrupting same-workspace viewport preservation.
Confidence Score: 4/5
The PR should not merge until the existing RestorePlannerTests snapshot initializer is updated to compile with the new required field.
The focus-workspace tracking is consistently propagated through the changed runtime paths, but adding the required snapshot property breaks an existing test-target initializer.
Files Needing Attention: Sources/Nehir/Core/Reconcile/ReconcileSnapshot.swift and Tests/NehirTests/RestorePlannerTests.swift
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR A[Managed focus confirmed] --> B[Store token and workspace] B --> C[Window reassigned] C --> D[AX focus confirmation] D --> E{Workspace unchanged?} E -- Yes --> F[Preserve viewport] E -- No --> G[Reveal focused window]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "Add tests for the workspace recorded wit..." | Re-trigger Greptile