Skip to content

fix(navigation): keep this device's own split its pick when following herdr's focus - #1887

Merged
devswha merged 2 commits into
mainfrom
fix/follow-focus-followups
Oct 24, 2026
Merged

devswha merged 2 commits into
mainfrom
fix/follow-focus-followups

Conversation

@devswha

@devswha devswha commented Oct 24, 2026

Copy link
Copy Markdown
Owner

What it does for the user

With Settings → Terminal → Follow herdr's focus on, opening a split from this device (Split pane right and open it, Split pane down and open it, or a pane's own Split right / Split down menu) always gives the new pane the keyboard and a Back entry to the pane it was split from. Before this, herdr's focus moving to the new pane could reach this device before the split's answer. This device then opened the new pane as an automatic pick, with no keyboard and no Back entry, and the split's answer, finding the selection already moved, did nothing.

The setting's description now also says that a PC on Ovis older than 0.4.13 does not report herdr's focus, instead of the setting silently doing nothing for such a PC.

Fixes #1470. #1803 already fixed the MEDIUM item and the regression-script cleanup; this fixes the two LOW items left.

Change

  • src/lib/followFocus.ts: movingHerdrFocus(machineId, request) marks a request from this device that moves herdr's focus on that PC, until the request settles (finally, which runs before the caller's own then). followsHerdrFocus gains ownMove: while such a request is out, that PC's pane-focused frames are not followed, and the caller opens the pane itself. Refusals end the hold too, and two overlapping requests hold until both are answered.
  • src/lib/api.ts: splitPane wraps the request when focus is set. It is the one path both split callers (App.tsx actions and PaneCanvas.tsx's menu) take, so neither caller changes. Other PCs' frames and other devices are unaffected.
  • src/lib/useMachineRoster.ts: passes ownMove with the frame's PC.
  • Settings description (en, ko, ja, zh), DESIGN.md's follow-focus entry and a changelog fragment. feat(navigation): follow herdr's focus on a device that asks to #1432 first shipped in v0.4.13 (remote bundle v32), hence the version.

Design precedent: none; the coding-agent apps have no outside focus to follow. It follows the rule followsHerdrFocus already has for a link still landing: an explicit action on this device wins over herdr's focus.

How it was verified

  • scripts/follow-focus-regression.ts (run by ui-regression.ts against an isolated herdr) has a new case. With the setting on, it opens Split pane right and open it from the command palette and holds the split's HTTP answer with page.route until herdr's frame for the new pane has been handled. The check for that is a tab created afterwards being listed. It then checks that the selection did not move, that the answer opens the new pane with the keyboard in it, and that Back returns to the pane it was split from.
    • On origin/main it fails: herdr's frame for this device's own split is left to the split's answer ('w3:pA' !== 'w3:p7'). The frame took the view first.
    • On this branch bun run check run --build bun scripts/ui-regression.ts passes, 72 PASS, including the existing follow-focus cases (off, on, typing, a dialog open).
  • src/lib/followFocus.test.ts: ownMove keeps the pane; the hold is per PC, is over inside the caller's then, ends on a refusal, and lasts until every overlapping request is answered (17 pass).
  • The Settings description was checked on the demo build at 1280 px (en dark, ko light) and 390 px (en light, ko dark, ja light, zh dark). Each names 0.4.13, the row stays inside its card, nothing scrolls sideways and there are no page errors. The screenshots were also looked at.
  • bun run check fast passes on this commit (typecheck, build, unit tests).
  • Reviewed by a code-review agent (omo) and Codex (gpt-6.1-sol). Both approved with LOW notes only:
    • Residual window: if the frame arrives after the split's answer but before React commits the caller's selection, the follow can still win. On main that window was the whole request; now it is the few milliseconds between the answer and the commit.
    • Ordering marker: the regression's marker tab and the focus frame come from separate herdr subscriptions, so if they ever swap the case can pass without proving anything. It cannot fail falsely, and it did fail on main as shown above.
    • Dropped frames: a frame from that PC while this device's split is out is dropped, not delayed. herdr moving elsewhere during that moment is not followed.

… herdr's focus

With Follow herdr's focus on, herdr's pane-focused frame for a split this device opened could
arrive before the split's answer and open the new pane as an automatic pick: no keyboard, no Back
entry, and the answer then found the selection moved. While this device's own focus-moving
request is out (movingHerdrFocus), that PC's frames are left to the request's caller. The
setting's description says a PC on Ovis older than 0.4.13 does not report herdr's focus.

Fixes #1470
@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between a575bd5 and 34659ac.

Files selected for processing: 11
  • DESIGN.md
  • changelog.d/follow-focus-own-split.md
  • scripts/follow-focus-regression.ts
  • src/components/SettingsDialog.tsx
  • src/lib/api.ts
  • src/lib/followFocus.test.ts
  • src/lib/followFocus.ts
  • src/lib/i18n/settings.ja.ts
  • src/lib/i18n/settings.ko.ts
  • src/lib/i18n/settings.zh.ts
  • src/lib/useMachineRoster.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

📝 Summary

Summary by CodeRabbit

  • Bug Fixes
    • When “Follow herdr’s focus” is enabled, opening a split on this device now keeps the new pane selected and focused, even if a focus update arrives before the split finishes. Back navigation returns to the original pane.
  • Documentation
    • The setting description now notes that PCs running Ovis versions earlier than 0.4.13 do not report herdr’s focus and should be updated.

Focused split requests now suppress focus-following for the requesting machine while pending. The regression script checks the selection, keyboard focus, and Back behavior when a focus frame arrives before the split response. Setting descriptions and documentation note the Ovis version limitation.

Changes: Own Split Focus Handling

Layer / File(s) Summary
Track and suppress own focus moves
src/lib/followFocus.ts, src/lib/api.ts, src/lib/useMachineRoster.ts, src/lib/followFocus.test.ts
Focused split requests are tracked per machine until they resolve or reject. Pane-focus events pass the pending-move state to the focus-follow decision, and tests cover own moves and overlapping requests.
Verify split behavior and document compatibility
scripts/follow-focus-regression.ts, DESIGN.md, changelog.d/follow-focus-own-split.md, src/components/SettingsDialog.tsx, src/lib/i18n/settings.*.ts
The regression script checks selection, keyboard focus, and Back behavior when a focus frame arrives before the split response. Documentation and setting descriptions note that Ovis versions before 0.4.13 do not report focus.

Priority: Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Low

Possibly related PRs

  • devswha/ovis#1803: Adds the busy input to the same focus-follow decision function and call site.
Merge Risk: Minimal · up to 34659

The split behavior and compatibility notes are ready for normal merge checks; no demonstrated issue requires a pre-merge fix.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 9 files. (2 skipped: 2… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: preserving this device's own split selection while following herdr's focus.
Description check Passed The description explains the problem, resulting behavior, implementation, issue references, and extensive validation results. It uses "How it was verified" instead of the template's "Validation" headi…
Linked Issues check Passed #1470 coding requirements are met. splitPane wraps focused split requests with movingHerdrFocus. useRosterEvents ignores matching PC focus frames until all overlapping requests settle, including…
Out of Scope Changes check Passed All changed files support #1470. The implementation changes focus handling, the regression test verifies the split race, and the unit tests cover request lifecycle behavior. DESIGN.md, the changelog, …
Full details: Description check

Explanation

The description explains the problem, resulting behavior, implementation, issue references, and extensive validation results. It uses "How it was verified" instead of the template's "Validation" heading, but the required validation information is present.

Full details: Linked Issues check

Explanation

#1470 coding requirements are met. splitPane wraps focused split requests with movingHerdrFocus. useRosterEvents ignores matching PC focus frames until all overlapping requests settle, including refusals. The caller then selects the new pane, which preserves keyboard focus and Back behavior. Tests cover this behavior, per-PC isolation, overlap, and refusal. The regression script checks the end-to-end ordering and resets the setting in finally. The English, Korean, Japanese, and Chinese descriptions state the Ovis 0.4.13 limit. The existing focus-follow guards and tests cover the local-interaction requirement.

Full details: Out of Scope Changes check

Explanation

All changed files support #1470. The implementation changes focus handling, the regression test verifies the split race, and the unit tests cover request lifecycle behavior. DESIGN.md, the changelog, and four setting translations document the same behavior. No unrelated change is demonstrated.

Full details: Docstring Coverage

Explanation

Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 9 files. (2 skipped: 2 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@devswha

devswha commented Oct 24, 2026

Copy link
Copy Markdown
Owner Author

Merge pass gate review of 34659ac: one blocker. An unanswered split request can switch off following herdr's focus with no end.

Blocker

  • The hold set by movingHerdrFocus is released only when the request settles (src/lib/followFocus.ts:44-51).
  • sendJson gives the request no client deadline (src/lib/api.ts:483-491).
  • If the PC never answers the split, for example a stalled connection that is neither refused nor closed, the request never settles. That PC's pane-focused frames are then never followed again on this device until a reload.
  • A probe on the real request confirmed the hold stays active and that fetch.signal is undefined.

Fix: bound the request, including reading the response body, for example with an AbortSignal.timeout on that call or on the hold itself. Please also add a test that shows the hold ends when the bound runs out.

Holds

  • A network error releases the hold.
  • Two overlapping requests don't release each other's hold early.
  • Another PC's frames are still followed.
  • A mutant that bypasses the ownMove guard fails the new assertion.
  • The PR makes no extra herdr focus call.
  • The design rationale for the setting text is in the PR.

The browser lanes are left to this PR's CI. The merge pass checks the fix commit (the changed range only) and then queues the PR.

…PC never answers

The merge pass found that a split request the PC never answers (a stalled connection, neither refused nor closed) kept that PC's pane-focused frames unfollowed until a reload. The hold now ends when the request settles or after OWN_MOVE_HOLD_MS, whichever comes first, and a late answer does not release a newer request's hold.
@devswha

devswha commented Oct 24, 2026

Copy link
Copy Markdown
Owner Author

Fixed the blocker in a4072c6: an unanswered split request no longer turns following off for that PC.

  • movingHerdrFocus (src/lib/followFocus.ts) now ends its hold when the request settles or after OWN_MOVE_HOLD_MS (10 s), whichever comes first. The timer is cleared when the request settles, and a held flag makes the release run once. So a late answer to a request whose bound already ran out cannot release a newer request's hold.
  • I bounded the hold rather than the request: aborting the split would not stop herdr from making the pane, and the caller would then never open it. A late answer still opens the new pane as this device's own pick, as before.
  • New test in src/lib/followFocus.test.ts (Bun fake timers, as api.test.ts uses): the hold is still on at 9 999 ms and off at 10 000 ms; a request made after that is not released by the stalled request's late answer, and ends with its own answer. Removing the bound and removing the once-only guard each fail it.
  • DESIGN.md says "for 10 s at most".

bun run check fast passes on a4072c6 (typecheck, build, unit tests: 18 in followFocus.test.ts). Only src/lib/followFocus.ts, its test and one DESIGN.md sentence changed; the browser case is unchanged.

@devswha

devswha commented Oct 24, 2026

Copy link
Copy Markdown
Owner Author

Merge pass review-work report. Overall: PASSED.

Lane Verdict Confidence
Gate review of 34659ac (a different model; programming + remove-ai-slops applied directly) REJECT: an unanswered split held follow-focus off until a reload HIGH
Merge pass re-check of a4072c6 (the fix) PASS -

The fix in a4072c6

  • movingHerdrFocus now releases its hold when the request settles or after OWN_MOVE_HOLD_MS (10 s), whichever comes first.
  • The release runs only once (held), and the timer is cleared when the request settles.
  • A late answer to a timed-out request doesn't end the hold of a request made after it.
  • Mutant: with the bound removed, the new fake-timer test fails.
  • 34 followFocus, api and conventions tests pass, and the typecheck passes.

Held from the gate

  • A network error releases the hold.
  • Overlapping requests don't release each other early.
  • Another PC's frames are still followed.
  • No extra herdr focus call is made.

Follow-up (LOW): the hold is bounded, but the stalled request itself is not. It stays pending in the browser until the connection gives up.

All required checks are green and no review thread is open.

CodeRabbit did not review this PR (rate-limited or skipped). The reviews above stand in for it.

@devswha
devswha merged commit adf79ff into main Oct 24, 2026
17 of 19 checks passed
@devswha
devswha deleted the fix/follow-focus-followups branch October 24, 2026 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Follow herdr focus: do not take the view while typing or in a dialog (#1432 follow-ups)

1 participant