cmux-next: a chat reopens where the reader left it on the host it actually opens on (cx-6bgzj)

Merged
TE
15d ago
+247-1

A chat reopens where the reader left it — on the host it actually opens on.

cx-6bgzj keeps a chat's place across a relaunch, and ChatScrollRestoreTests holds that promise. On the shared page host. AgentPaneTunables.pageHost ("agent.pageHost") defaults to false, so an agent pane's page talks to the pane's own host (AgentPaneBridge), and that is the path every reader is on today. Nothing there had a route for chat.scrollAnchor or chat.scrollAnchor.get: AgentPaneRequest decodes both as .unsupported, and the page swallows the refusal — fetchScrollAnchor catches to null, postScrollAnchor catches and drops it — so the per-chat store was never written or read on that path.

A tab open at a clean quit still came back where it was: its place rides the handshake's wake (AgentPaneWakeState.scrollAnchor, collected by QuitCoordinator, handed back at launch), which reaches window.cmuxAcpmuxActions directly and needs neither of these two ops. What was lost is the chat's durable place — after a crash, in a second tab showing the same chat, and in a tab closed and opened again, the chat reopened at its newest message. The first version of this description overstated that as "no place was ever kept"; the review below caught it.

I found this while doing the new-tab tail cache (#23131), where agent.pageHost being off is why that change had to serve both hosts.

Why nothing caught it

webviews/test/agent-pane-reading-place.test.ts drives the real pane page and asserts the chat comes back at the reader's row — but it stubs window.cmuxAcpmuxActions["chat.scrollAnchor.get"], which stands in for the host. It tests the page's half, and the page's half works. The Swift half had one path and needed two.

The change

One store, both hosts. AgentPaneScrollAnchors+Bridge.swift answers the two ops from AgentPaneScrollAnchors.shared — the same store AgentPageProvider+Scroll reads — and the bridge routes them there before AgentPaneRequest, so either host keeps and hands back what the other wrote. It stays out of AgentPaneModel exactly as it does on the page host and as chat.tail does, and the bridge does not log it, because it names a row of the person's transcript. A host with no store (a test host, the gallery) answers "no place" instead of refusing, so a page there never waits on a failure.

The same shape as #23131's tail cache, which is what the ops and the two paths already asked for.

Red before green

A greenfield op cannot be red on a base that will not compile against the test, so the first commit writes down what that host does today — the seam returning the same unsupported failure AgentPaneRequest + the model produce for these methods — with the bridge routed to it. The two behavior tests are red against that, and the second commit changes only the seam's body. A reviewer can check the first commit against AgentPaneRequest+Decoding.swift to confirm it is faithful.

Changelog

Fixed: a chat reopens where you left it reading, instead of jumping to its newest message.

No capture: what changes is where a reopened chat is scrolled to, which is a before/after of the same transcript at two scroll offsets; the red-then-green fleet runs below are the evidence. (A tour still of a chat mid-scroll would not show which commit it came from.)

The red and the green

Both are fleet runs of scripts/ci/package-test-lane.sh suite Packages/macOS/CmuxNext (Swift never runs on a contributor's Mac or on big-red).

Red, at the test commit 2631367cbe1b1 (aws-m4pro-7) — the two behavior tests fail, and the refusal test passes, so it is the capability that is missing and not the test:

✘ aChatReopensWhereItRestedWithThePageHostOff() ... Expectation failed: (reply["ok"] as? Bool → false) == true
✘ ... :43: Expectation failed: (place?["rowId"] as? String → nil) == "user-42"
✘ ... :44: Expectation failed: (place?["offset"] as? Double → nil) == 120.5
✘ thePageHostAndThePanesOwnHostShareTheOneStore() ... :72: (read → .null) == .object(["rowId": .string("user-7"), "offset": .number(8)])
✔ aRequestWithoutAChatIsRefusedAndNothingIsKept() passed
✘ Test run with 3 tests in 1 suite failed ... with 13 issues

place?["rowId"] → nil is the bug in one line: the chat's place was written and the host had nothing to give back.

Green, at the fix 681e18a8a1443 (aws-m4pro-4), with the page host's existing suite run beside it to show it did not move:

✔ Suite ChatScrollRestoreDefaultPathTests passed after 0.007 seconds.   (3 tests)
✔ Suite ChatScrollRestoreTests passed after 0.004 seconds.              (2 tests)
ChatScrollRestoreDefaultPathTests   passed   8s
ChatScrollRestoreTests              passed   1s
All 2 suites passed.

Green again on the current head ce237f908d13a, which is four commits past
that fix: cmux-ci key chatplace-green-5, the same two suites. The head's
earlier run (ca4f8b9be91c61ef9b6cda54, key chatplace-green-3) was All 2 suites passed over 6 tests. The result of the new one goes in a comment below,
green or not, before this merges; nothing here claims it has returned.

What three reviews changed after that green

The implementation moved after 681e18a8a1443, so the green above it is not the
head's:

  • The seam came out of the handler. AgentPaneBridge.chatPlaceReply(for:) is
    now the handler's whole chat-place branch, taking the same message.body, and
    the tests drive that.
  • The page host stopped prefix-matching. AgentPageProvider.call used
    method.hasPrefix("chat.scrollAnchor"); scrollAnchor treats anything but
    .get as a write, so a later chat.scrollAnchor.forget would have erased a
    chat's place instead of being refused. It now asks
    AgentPaneScrollAnchors.handles(method:), which names the two ops. No op in
    AgentPageOps.methods has that prefix today, so this refuses nothing that used
    to work.
  • reply checks the method before the store, so a refusal does not depend on
    a store existing.
  • The one call site is now held in the source. WKScriptMessage has no
    public initializer, so no test can send the handler a message: the second and
    third reviews each found that deleting the handler's branch left every test
    green while a chat silently stopped keeping its place.
    theHandlerStillCallsThatSeam reads the call out of AgentPaneBridge.swift,
    inside the handler rather than anywhere in the file. Checked both ways: green
    as committed, red with the branch deleted.

Corrections I owe this description

  • A test doc comment and a commit message said deleting the handler's call turned
    the suite red. That was false — and it was the sentence the next person
    would have read before deleting the branch. Fixed in ce237f908d13a, which is
    also what the source-reading test is for.
  • reply's reordered-guard comment claimed it fixed a live refusal. Both callers
    already filter by handles, so nothing in the app reaches that guard; the
    comment now says so and why the guard is still worth keeping.

Observed / Not run

The fleet runs named above, each at the commit named. prepush-gate.py: no new
violations, on every commit. lint-ios-package-conventions.sh --namespace-fix:
no violation in this PR's files; its one ERROR is base debt in
CmuxHomeCore/HomeThoughtHeadline.swift, which the fixer rewrote twice and I
reverted both times so it is not in this diff. The mutation above was checked by
running the extraction over the real file with and without the branch.

Base reds, none of them this PR's: cmux-next checks is failure on base
commits 34ab7d84e47b and e187ba943470; its named red is Tier routing, from
a ci-target-graph.json that lacks CmuxNextHomeEngineTests — a directory that
already existed at this branch's point, which base regenerated afterwards. #23260
owns that fix. The CI guard group annotation names "Validate scripts keep full
clones full"; this PR changes no script, tests/test_ci_no_shallow_dev_fetch.py
passes locally, and #23234 carries the identical annotation at its own head.

Not run: the rest of the Swift suite, and the page's own half, which is unchanged — agent-pane-reading-place.test.ts already covers it and this PR does not touch a line of TypeScript.

🤖 Generated with Claude Code
Agent session: big-red

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

Summary by cubic

A chat now reopens where the reader left it, on the host it actually opens on, not just the shared page host.

agent.pageHost is off by default, so an agent pane's page talks to its own host (AgentPaneBridge), which previously had no route for chat.scrollAnchor and chat.scrollAnchor.get — the page swallowed the refusal, so a chat reopened at its newest message after a crash, in another tab, and in a tab closed and reopened. The bridge now routes the two ops to the shared AgentPaneScrollAnchors store, the same one AgentPageProvider+Scroll reads on the page host, so both hosts keep and hand back the same chat place.

  • Both hosts match the ops by name, not prefix, so a later chat.scrollAnchor… op reaches the model instead of being swallowed or erasing a place.
  • A host with no store (test host, gallery) answers "no place", and an unserved op is refused regardless of whether a store exists.
  • Tests drive the handler's whole bridge branch (chatPlaceReply), and a source-reading test holds the handler's call site, since a WKScriptMessage cannot be built in a test. Comments name only ops and callers that actually exist.