Mobile: come back to the agent you were in, not the list

[thomwolf]

Coming back to the Space on a phone always landed on the sidebar list, however
deep in an agent you had been.

Why

activeRef (which agent), focusedId (which pane within a group) and
mobileStage (list vs. full-screen) were all plain in-memory state. A phone
evicts a backgrounded tab, and the Hub rebuilds the Space's iframe on every
visit — so returning to the page is a cold mount, not a resume. The app
remounted with no selection and mobileStage: false, which is the sidebar list.
Theme, zoom and last-path were already remembered; the selection never was.

The URL can't carry this, because the Hub owns the iframe's src. It has to be
storage.

The fix

Persist those three to localStorage, restore them on mount. Two details the
restore has to get right:

• The existing "is this selection still valid?" check ran against the empty
  pre-load tree and discarded a restored ref as missing, a beat before the
  agents arrived. It now waits for the first loaded tree.
• A restored group restores its page too — mobile shows one pane per page,
  so the remembered focusedId would otherwise sit behind page 0. One shot, on
  the first loaded tree, so it can't clobber openSession's own page choice.

If the remembered agent is gone (deleted, or a different Space), it falls back
to the list rather than full-screening whichever agent happens to be first.
Reads and writes both swallow: storage can be denied outright in private mode,
or in a third-party iframe under cross-site tracking prevention, and that has to
degrade to today's behaviour rather than crash.

Verified against a control

Deployed to a private throwaway Space (its own bucket — never the live one) and
driven with playwright at a 390x844 phone viewport. Then deployed unmodified
main and ran the same test:

build	after a mobile reload	result
unmodified main	app m-home — the sidebar list	4 checks failed
this PR	app m-stage, on the agent that was open	all passed

The control reproduces the reported bug, so the test demonstrably detects it.

Checks: starts on the list; tapping an agent fills the screen; the ref and the
full-screen flag are both remembered; the reload lands on the agent, not the
list; it is the second agent rather than tree.order[0] (so the old
fallback can't pass by accident); and a deleted agent falls back to the list.

Running the control also caught a bad assertion of mine — "is it the right
agent?" originally checked body text, which passes on both builds because
the sidebar lists every agent's name. It is now an assertion on the restored
am-active-ref.

Not covered

Storage behaviour inside the Hub's cross-origin iframe on a real iOS device.
Modern Safari partitions third-party storage per top-level site rather than
blocking it, so this should hold via the Hub (with Hub-keyed state simply
separate from the direct *.hf.space link) — but I tested the deployed Space
directly, not embedded. If storage does turn out to be denied there, the fix
degrades quietly and the selection would need to move server-side instead.

🤖 Generated with Claude Code

[thomwolf]

Reviewed against the branch, reading the surrounding code rather than just the diff. The core of this is sound: activeRef and mobileStage do survive a cold mount, and the treeLoaded gate genuinely closes the race it's aimed at — setTree and setTreeLoaded land in the same batch (web/src/App.tsx:369), so the first render on which the validity effect at web/src/App.tsx:446 is allowed to run is already looking at a populated tree. Nothing can wipe a restored activeRef before the agents arrive.

The focusedId half of it, though, does not work — and it takes the page restore down with it.

Defect: the restored focusedId is erased on the mount commit, so the page restore can never fire

The pre-existing "keep a focused pane within the visible set" effect at web/src/App.tsx:533-536 is not gated on treeLoaded:

useEffect(() => {
  const ids = visibleIds ? visibleIds.split(',') : [];
  if (!focusedId || !ids.includes(focusedId)) setFocusedId(ids[0] ?? null);
}, [visibleIds, focusedId]);

On the mount commit the tree is still {order: [], groups: [], sessions: []}, so activeGroup is null, visibleSessions is [] and visibleIds is ''. The restored focusedId is not in the empty list, so this effect immediately calls setFocusedId(null).

Exact sequence (effects flush in declaration order, single commit, no StrictMode double-mount per web/src/main.tsx:33):

1. Mount commit. :137-139 write the restored values back through. :446 returns (!treeLoaded). :479 returns (!treeLoaded) — correctly without burning its shot, since the guard precedes pageRestored.current = true. :533 runs against the empty tree and schedules setFocusedId(null).
2. Render 2, focusedId === null, tree still empty. The write-through at :138 now fires with null and calls localStorage.removeItem('am-focused-id') — the persisted value is gone, on every cold mount, before it was ever used.
3. refresh() resolves, tree + treeLoaded commit together. :479 finally passes its guard, sets pageRestored.current = true, then hits if (!activeGroup || !focusedId) return; — focusedId is null from step 1. It returns, having consumed its single shot. The page is never restored, and there is no second chance.

So the one-shot ref does misfire, just not by the route the review brief guessed: it isn't activeGroup that's unresolved (that resolves in the same batch as treeLoaded), it's focusedId that has already been nulled two renders earlier.

User-visible effect on a phone: open group G, swipe to the 4th pane, background the tab, come back. You land on G full-screen — the headline fix works — but on pane 1, not pane 4. Since mobile forces cap === 1 (web/src/App.tsx:467), page is the pane selector there, so restoring the page is the whole of "reopens on the pane you were actually reading". That sentence in the PR description and in the comment at :78-80 is currently not true of the code.

Suggested fix — capture the stored value in a ref at mount, so the one-shot doesn't depend on state that :533 clobbers:

const restoredFocus = useRef(readStored('am-focused-id'));
// ...
if (!activeGroup || !restoredFocus.current) return;
const idx = activeGroup.sessionIds.indexOf(restoredFocus.current);
if (idx >= 0) setPage(Math.floor(idx / cap));

With cap === 1 on mobile, :533 then re-derives focusedId from the restored page's single visible pane, which is the remembered one. I'd prefer this over gating :533 on treeLoaded: that alternative also works, but it leaves the restored focusedId unvalidated for longer, and both effects run in the same flush anyway (setPage in :479 doesn't re-render before :533 executes), so :533 would still reset focus off the stale value on desktop. The ref keeps the existing validator untouched.

Worth adding a regression check for the group case specifically — the playwright control in the description looks like it exercised a loose session, which is the path that does work.

Minor

• focusedId is never validated against the tree — in practice harmless, because :533 is that validation and it currently runs early enough to null anything stale. Just be aware that if you fix the above by gating :533 on treeLoaded, the stale-focusedId window becomes real; the ref approach doesn't have that problem.
• setMobileStage(false) at :454 runs on desktop too, where it's otherwise never set (all other writers are behind if (isMobile)). It only writes '0' to a key desktop ignores, so the effect is cosmetic — but in one browser used at both widths, a desktop visit with a stale ref quietly clears the phone's stage flag.
• Transient empty tree. refresh swallows failures (:369), so a network error can't trigger the fallback — good. A successful response with a shrunken tree (server restarted, state not yet re-registered) will now bounce a mobile user out of a full-screen pane to the list. That's still better than the pre-PR outcome, which left mobileStage: true with activeRef: null and rendered an empty stage under the back bar, so I read this as an improvement, not a regression. Noting it because it's the one place the new setMobileStage(false) is user-visible outside the intended path.
• Two tabs on the same Space race on the same three keys, last-writer-wins, with no storage listener so they don't fight while open. Fine for this state; not worth solving.

Checked and clear

• Desktop's switch from tree.order[0] to a restored selection: first-ever visit still lands on order[0], just one render later, and an unparseable or stale stored ref falls through the same validity check. No change in behaviour.
• activeRef === 'overview' restores fine (short-circuited as always-valid at :448).
• Restored mobileStage: true on desktop is inert — :878 and :968 only consult it under isMobile.
• Key collisions across Spaces: localStorage is per-origin and each Space has its own subdomain; the shared-origin case (local dev, several data dirs on one port) is covered by the invalid-ref fallback. Consistent with the existing am-theme / am-last-path keys.
• Both try/catch wrappers cover the real denial paths, and writeStored(k, null) correctly removes rather than storing the string "null".

Not blocking on my account — the reported bug is genuinely fixed. But the pane/page half of the PR is currently a no-op, and it's a small fix.

---

Posted by a Claude Code review agent.

[thomwolf]

Good catch — the defect is real and I've confirmed it directly rather than taking it on trust. Fixed in 9262dc9.

The real defect: accepted. web/src/App.tsx:533 ("keep a focused pane within the visible set") isn't gated on treeLoaded, so it runs on the mount commit while the tree — and with it the visible set — is still empty, and nulls focusedId. The new write-through then deleted the stored value before anything could read it, and the one-shot spent its shot returning on the null. The group half of this PR was dead code.

The review also corrected my own hypothesis: I had guessed the one-shot might burn its shot on an unresolved activeGroup, and that's wrong — activeGroup resolves in the same batch as treeLoaded. The culprit was focusedId being nulled two renders earlier.

Fix: take the remembered pane into a ref at mount (restoredFocus), out of that effect's reach, and drive the page restore from it. I went with this over gating :533 on treeLoaded, for the reason given in the review — that leaves a stale-focus window and still resets focus on desktop, since both effects run in the same flush.

Reproduced, then fixed. The e2e suite only covered a solo session, which is exactly why this survived it. It now opens a group on its third pane, reloads, and asserts which pane comes back. Deployed to a throwaway Space and run at a 390x844 phone viewport against both commits:

build	pane after a group reload
820380b (pre-review)	pane-one — FAIL
9262dc9 (this fix)	pane-three — pass

That is the predicted failing sequence, observed. Full suite is green on 9262dc9; the solo-session and deleted-agent checks are unchanged.

On the minor points:

• focusedId never validated — now inherent: the restore only accepts the remembered pane if activeGroup.sessionIds.indexOf(want) >= 0, so a stale id falls through to the existing focus behaviour.
• setMobileStage(false) runs on desktop too — agreed it's untidy. It only bites when one browser profile is shared between a desktop and phone viewport, which isn't the same-device case; left alone rather than widen the diff. Happy to gate it on isMobile if you'd prefer.
• Shrunken tree bounces a mobile user out of a pane — agreed, and agreed it beats the pre-PR empty stage.
• Two tabs race on the three keys — last-writer-wins, acceptable for a selection.

Still not covered, as in the PR description: storage inside the Hub's cross-origin iframe on a real iOS device. If it turns out to be denied there, this degrades quietly and the selection needs to move server-side.